Rector: Symfony 4.3 config level wrongly modifies the dispatch() method call

[Aerendir]

ERROR 1: event object already passed with a name for the event

    ---------- begin diff ----------
--- Original
+++ New
@@ -66,7 +66,7 @@

             if (empty($icons)) {
                 // No favicon found: skipping...
-                $this->getEventDispatcher()->dispatch($event, FaviconEvent::NO_FAVICON);
+                $this->getEventDispatcher()->dispatch($event);

                 return;
             }
@@ -76,7 +76,7 @@
             $tmpFaviconFile     = $this->getFaviconManager()->download($faviconUrl);
             $tmpFaviconFileHash = LocalFileManager::hash($tmpFaviconFile);
         } catch (\Throwable $e) {
-            $this->getEventDispatcher()->dispatch($event, FaviconEvent::DOWNLOAD_ERROR);
+            $this->getEventDispatcher()->dispatch($event);

             return;
         }
@@ -85,7 +85,7 @@
         $currentFavicon = $event->getChannel()->getIcon();
         if (false === empty($currentFavicon) && $currentFavicon['hash'] === $tmpFaviconFileHash) {
             // Favicon didn\'t changed: skipping
-            $this->getEventDispatcher()->dispatch($event, FaviconEvent::UNCHANGED);
+            $this->getEventDispatcher()->dispatch($event);

             return;
         }
    ----------- end diff -----------

Applied rectors:

 * Rector\Php\Rector\FuncCall\RemoveExtraParametersRector

In this case FaviconEvent::UNCHANGED is the name of the event and it is allowed to be passed.

There is more: this causes a complete break in the application as the same event passed with different names, now is always passed as is making impossible for the Subscriber to understand the purpose of the event:

EXPECTED BEHAVIOR: ignore the lines that pass the event as first argument and an event name as second argument.
Uncertain on what to do: what if the second argument is like FaviconEvent::class?

ERROR 2: Event name using a constant as a first argument and event object as second argument

    ---------- begin diff ----------
--- Original
+++ New
@@ -61,7 +61,7 @@
     public function import(Channel $channel)
     {
         $faviconEvent = new FaviconEvent($channel);
-        $this->getEventDispatcher()->dispatch(FaviconEvent::DOWNLOAD, $faviconEvent);
+        $this->getEventDispatcher()->dispatch(FaviconEvent::DOWNLOAD);
     }

     /**
@@ -101,7 +101,7 @@
     {
         // Download the image file from the remote channel
         $faviconDownloadEvent = new FileDownloadEvent($faviconUrl);
-        $this->getEventDispatcher()->dispatch(FileDownloadEvent::DOWNLOAD, $faviconDownloadEvent);
+        $this->getEventDispatcher()->dispatch(FileDownloadEvent::DOWNLOAD);

         return $faviconDownloadEvent->getTmpFile();
     }
@@ -115,7 +115,7 @@
     {
         $uploadImageEvent = new FileUploadEvent($uploadingFile, LocalFileManager::UP_TYPE_CHANNEL_ICON, $channel);
         $uploadImageEvent->setUploadName('icon');
-        $this->getEventDispatcher()->dispatch(FileUploadEvent::UPLOAD, $uploadImageEvent);
+        $this->getEventDispatcher()->dispatch(FileUploadEvent::UPLOAD);

         $channel->setIcon([
             'name' => $uploadImageEvent->getUploadedFilename(),
    ----------- end diff -----------

Applied rectors:

 * Rector\Php\Rector\FuncCall\RemoveExtraParametersRector

In this case, the event is completely removed, passing only the event name.

EXPECTED BEHAVIOR: Considering that is allowed a second parameter containing the name of the event, I expect a change like this:

-        $this->getEventDispatcher()->dispatch(FileUploadEvent::UPLOAD, $uploadImageEvent);
+        $this->getEventDispatcher()->dispatch($uploadImageEvent, FileUploadEvent::UPLOAD);

ERROR 3: Event object and event name as a variable

    ---------- begin diff ----------
--- Original
+++ New
@@ -78,6 +78,6 @@
         $event->setLocalPurchase($localPurchase);

         $finalEventName = $needsUpdate ? RemotePurchaseEvent::PROCESSED : RemotePurchaseEvent::SKIPPED;
-        $this->getEventDispatcher()->dispatch($finalEventName, $event);
+        $this->getEventDispatcher()->dispatch($finalEventName);
     }
 }
    ----------- end diff -----------

Applied rectors:

 * Rector\Php\Rector\FuncCall\RemoveExtraParametersRector

As you can see, in this snippet $finalEventName is a string and is not a class but something like event_name while the second object is the real event passed.

EXPECTED BEHAVIOR: I expect something like what I expect in ERROR 2:

-        $this->getEventDispatcher()->dispatch($finalEventName, $event);
+        $this->getEventDispatcher()->dispatch($event, $finalEventName);

CONCLUSIONS

Before submitting failing test cases I'd like to first know what do you think @TomasVotruba .

[TomasVotruba]

Thanks for detailed report.

As you suggest, this is desired behavior:

-        $this->getEventDispatcher()->dispatch(FileUploadEvent::UPLOAD, $uploadImageEvent);
+        $this->getEventDispatcher()->dispatch($uploadImageEvent, FileUploadEvent::UPLOAD);

Please send test case.

Also, in some next rule it should be possible this:

-->dispatch(FileUploadEvent::UPLOAD)
+->dispatch(FileUploadEvent::class)

  publci function getSubscribedEvents() {
-   return [FileUploadEvent::UPLOAD => 'someMethod'];
+   return [FileUploadEvent::class => 'someMethod'];
  }

With such combination, the constant UPLOAD can be dropped. It will be useless in the future Symfony anyway.

[Aerendir]

It will be useless in the future Symfony anyway.

But it will be anyway explicitly allowed:

he signature of the EventDispatcherInterface::dispatch() method has been updated to dispatch($event, string $eventName = null)

Source: https://github.com/symfony/symfony/blob/4.4/UPGRADE-5.0.md#eventdispatcher

(And, BTW, I will continue to use it as it will not force me to create many different classes with the same exact values 😅).

[TomasVotruba]

(And, BTW, I will continue to use it as it will not force me to create many different classes with the same exact values sweat_smile).

:+1: Sure, I didn't mean to enforce behavior that will add work, but rather keep BC compatible code without any work.

[TomasVotruba]

Ah, now I see it's RemoveExtraParametersRector. That's caused probably by commented out Symfony magic.

What does the dispatch() method body look like?

[TomasVotruba]

If you manage to set failing test to RemoveExtraParametersRectorTest, it would be the best :+1:

[Aerendir]

Oh damn, I forget the test case... Tomorrow I will send it!

[gnutix]

@Aerendir Tomorrow: the day that never comes! :notes: ;)

[Aerendir]

@gnutix , excuse me for the late response: I had a very bad flu (38+ 😓). Monday I will create and submit the test...

[TomasVotruba]

@Aerendir Just relax :+1: , you don't have to sacrifice your life to create a test case. We'll have more challanging tasks in the future and health is always the priority.

---

As for future sentences, I'd like to share my personal approach I've learned in last 8 years doing open-source.

99 % of promises works only in movies and fizzles out. It's artificial responsibility for the maintainer and for the author. No-one really knows what that means.

What I've learned to do in open-source (and relationships in general) is to speak about future only when I'm 100 % sure - like "we'll have lunch on Monday at 12:00 in Vltavska street".

If I have a plan, I'll say "I might do it, but don't count on me". But better "" (nothing), because that's the real information value, right?

Then I write "I'm wroking on it", while I've really started to work on it. I might work on someting for 3 days, but if no-one knows, there might be 10 people working on it at the same time - it happened to me many times and it can turn joy to depression very quickly.

This way everyone has real information that can work with and plan their own time :)

I've also learned (hard way) to say "no, I don't have time for this at the moment because of other project took higher priority; I'm sorry if that affected your planning; I though I could make it, but I overestimated myself".

I think setting boundaries and setting them in emphatic way is very important for healthy (open-source) life.

Well, that's 2 cents of life "wisdom" from me :P

[TomasVotruba]

All discribed cases are tested and covered now. It's seems finished now.

If anyone will have similar issue, please send faling test case to particulare Rector rule, so we have it covered.

[Aerendir]

Damn, I've just opened PHPStorm to work on this 😓