Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-12016

Event listeners should be services

    Details

    • Type: Bug
    • Status: Unverified Fix
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.1.0-a1
    • Fix Version/s: 3.1.0-a2
    • Component/s: Events
    • Labels:
      None

      Description

      They should either be provided from a provider, or we should check whether there is a service available for that class, instead of creating the instance ourselves.
      Current code:

      		foreach ($subscriber_classes as $class)
      		{
      			$subscriber = new $class();
      			$this->dispatcher->addSubscriber($subscriber);
      		}

        Activity

        Hide
        imkingdavid David King added a comment -

        If we're going to change it, I vote we make them services and then use a Service Collection (as described here: https://www.phpbb.com/community/viewtopic.php?f=461&t=2210001) to grab them all.

        That being said, using the Finder in this case allows us to enforce a directory structure (i.e. the files must be in the event/ directory). Using a Service Collection will allow the file to be practically anywhere in an extension, meaning that in theory some extensions could lack much organization.

        Show
        imkingdavid David King added a comment - If we're going to change it, I vote we make them services and then use a Service Collection (as described here: https://www.phpbb.com/community/viewtopic.php?f=461&t=2210001 ) to grab them all. That being said, using the Finder in this case allows us to enforce a directory structure (i.e. the files must be in the event/ directory). Using a Service Collection will allow the file to be practically anywhere in an extension, meaning that in theory some extensions could lack much organization.
        Hide
        nickvergessen Joas Schilling added a comment -

        @imkingdavid yes, we loose the force option, but the benefit is better then loosing this.
        Also if we do not document that it could be anywhere, people may just do it like all demo's do it

        On the otherhand, if you want to force it, we could also use the patch i just provided combined with the initial implementation and only add the listener, when its class has also been found by the finder. But I'd say, not worth the effort

        Show
        nickvergessen Joas Schilling added a comment - @imkingdavid yes, we loose the force option, but the benefit is better then loosing this. Also if we do not document that it could be anywhere, people may just do it like all demo's do it On the otherhand, if you want to force it, we could also use the patch i just provided combined with the initial implementation and only add the listener, when its class has also been found by the finder. But I'd say, not worth the effort

          People

          • Assignee:
            nickvergessen Joas Schilling
            Reporter:
            nickvergessen Joas Schilling
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development