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

The prefix for the notification's services is harcoded

    Details

    • Type: Bug
    • Status: Unverified Fix
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.1.0-RC3
    • Fix Version/s: 3.1.0-RC4
    • Component/s: Notification System
    • Labels:
      None

      Description

      The prefix for the notification's services (notification.type.) is hardcoded in the manager. Instead we should use the full service name (so without any prefix), which will allow the extensions to give a proper name to their notifications. (it is the same for the notifications method)

        Issue Links

          Activity

          Hide
          nicofuma nicofuma added a comment - - edited

          I looked quickly at the code and I think that we need to throw an exception in both get_item_type_class and get_method_class and then handle this exception in 6 or 7 methods. But I'm not sure that it is the best solution, because it needs a lot of changes to handle a very special case (ie: migration from RC3 to RC4) and at the same time it will hide all the errors thrown during the development of the extensions (but they still be available in the logs). So I don't know if it's the best way... maybe it's better to handle this case in the migration by disabling all the other notifications like VSE said.

          btw: it could be a good thing to catch and log as many errors like that as possible... maybe it could be a discussion for 3.2 or after (regardless the decision taken here)

          Show
          nicofuma nicofuma added a comment - - edited I looked quickly at the code and I think that we need to throw an exception in both get_item_type_class and get_method_class and then handle this exception in 6 or 7 methods. But I'm not sure that it is the best solution, because it needs a lot of changes to handle a very special case (ie: migration from RC3 to RC4) and at the same time it will hide all the errors thrown during the development of the extensions (but they still be available in the logs). So I don't know if it's the best way... maybe it's better to handle this case in the migration by disabling all the other notifications like VSE said. btw: it could be a good thing to catch and log as many errors like that as possible... maybe it could be a discussion for 3.2 or after (regardless the decision taken here)
          Hide
          Marc Marc added a comment -

          Am I correct in the assumption that up to this PR every notification type was prefixed with notification.type.? If yes, why don't we just add that prefix to any notification type that doesn't have it?

          I am however not sure how an extension would be able to rename their service if they wanted to?

          Show
          Marc Marc added a comment - Am I correct in the assumption that up to this PR every notification type was prefixed with notification.type.? If yes, why don't we just add that prefix to any notification type that doesn't have it? I am however not sure how an extension would be able to rename their service if they wanted to?
          Hide
          nicofuma nicofuma added a comment -

          it's what was done before, but we could add the prefix if the notification type does not contains any dot. And we will mark these changes as "Be removed in 3.1.0 final" (or 3.2.0)

          Show
          nicofuma nicofuma added a comment - it's what was done before, but we could add the prefix if the notification type does not contains any dot. And we will mark these changes as "Be removed in 3.1.0 final" (or 3.2.0)
          Hide
          VSE Matt Friedman added a comment -

          That could work. It could at least give ext authors time to update their ext without worrying about BC breaks.

          Show
          VSE Matt Friedman added a comment - That could work. It could at least give ext authors time to update their ext without worrying about BC breaks.
          Hide
          nickvergessen Joas Schilling added a comment - - edited

          just add the prefix if the string does not contain dots sounds like a good idea.
          This way it still works and ext authors can update it accordingly

          And please change the existing migration for that. (And we do not need to change that later at any time)

          Show
          nickvergessen Joas Schilling added a comment - - edited just add the prefix if the string does not contain dots sounds like a good idea. This way it still works and ext authors can update it accordingly And please change the existing migration for that. (And we do not need to change that later at any time)

            People

            • Assignee:
              nicofuma nicofuma
              Reporter:
              nicofuma nicofuma
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development