Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.2][Events] Use event classes for Extension plugins #43617

Merged
merged 8 commits into from
Aug 28, 2024

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Jun 4, 2024

User description

Summary of Changes

Update extension and installer plugins to use SubscriberInterface event classes.

Testing Instructions

Test that extension installation works as before.
Joomla update also works as before.

Actual result BEFORE applying this Pull Request

Works

Expected result AFTER applying this Pull Request

Works

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:
  • No documentation changes for docs.joomla.org needed
  • Pull Request link for manual.joomla.org: New Event classes Manual#177
  • No documentation changes for manual.joomla.org needed

PR Type

Enhancement


Description

  • Updated extension and installer plugins to use SubscriberInterface event classes.
  • Introduced AbstractJoomlaUpdateEvent class and its concrete implementations BeforeJoomlaUpdateEvent and AfterJoomlaUpdateEvent.
  • Modified Joomla update process to dispatch new event classes.
  • Implemented SubscriberInterface for various extension plugins and updated their methods to use event classes.

Changes walkthrough 📝

Relevant files
Enhancement
9 files
UpdateModel.php
Use event classes for Joomla update process                           

administrator/components/com_joomlaupdate/src/Model/UpdateModel.php

  • Added dispatching of BeforeJoomlaUpdateEvent and
    AfterJoomlaUpdateEvent instead of triggering events directly.
  • Included new event classes for Joomla update process.
  • +8/-3     
    CoreEventAware.php
    Map Joomla update events to core events                                   

    libraries/src/Event/CoreEventAware.php

  • Added onJoomlaBeforeUpdate and onJoomlaAfterUpdate to the core event
    mapping.
  • +2/-0     
    AbstractJoomlaUpdateEvent.php
    Introduce abstract class for Joomla update events               

    libraries/src/Event/Extension/AbstractJoomlaUpdateEvent.php

  • Introduced abstract class AbstractJoomlaUpdateEvent for Joomla update
    events.
  • +55/-0   
    AfterJoomlaUpdateEvent.php
    Add AfterJoomlaUpdateEvent class                                                 

    libraries/src/Event/Extension/AfterJoomlaUpdateEvent.php

  • Added AfterJoomlaUpdateEvent class extending
    AbstractJoomlaUpdateEvent.
  • +58/-0   
    BeforeJoomlaUpdateEvent.php
    Add BeforeJoomlaUpdateEvent class                                               

    libraries/src/Event/Extension/BeforeJoomlaUpdateEvent.php

  • Added BeforeJoomlaUpdateEvent class extending
    AbstractJoomlaUpdateEvent.
  • +23/-0   
    Finder.php
    Implement SubscriberInterface for Finder plugin                   

    plugins/extension/finder/src/Extension/Finder.php

  • Implemented SubscriberInterface for the Finder extension plugin.
  • Updated methods to use event classes.
  • +35/-13 
    Joomla.php
    Implement SubscriberInterface for Joomla plugin                   

    plugins/extension/joomla/src/Extension/Joomla.php

  • Implemented SubscriberInterface for the Joomla extension plugin.
  • Updated methods to use event classes.
  • +36/-13 
    NamespaceMap.php
    Implement SubscriberInterface for NamespaceMap plugin       

    plugins/extension/namespacemap/src/Extension/NamespaceMap.php

  • Implemented SubscriberInterface for the NamespaceMap extension plugin.
  • Updated methods to use event classes.
  • +30/-15 
    Override.php
    Implement SubscriberInterface for Override plugin               

    plugins/installer/override/src/Extension/Override.php

  • Implemented SubscriberInterface for the Override extension plugin.
  • Added event subscription methods.
  • +21/-1   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    qodo-merge-pro bot commented Jun 4, 2024

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and changes related to event handling in a complex system like Joomla. Understanding the context and ensuring that the new event classes integrate seamlessly with the existing system requires a moderate level of effort.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Duplicate Event Dispatch: In UpdateModel.php, the AfterJoomlaUpdateEvent is dispatched twice in close succession within the cleanUp method. This could lead to unintended side effects or duplicate processing. One of these dispatches should likely be removed to prevent potential issues.

    🔒 Security concerns

    No

    @Fedik Fedik added the Feature label Jun 4, 2024
    Copy link

    qodo-merge-pro bot commented Jun 4, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Change the parameter type to AfterInstallEvent to match the event being dispatched

    The method onExtensionAfterInstall is defined to accept an AbstractExtensionEvent
    parameter, but it should accept an AfterInstallEvent parameter to match the event being
    dispatched.

    plugins/extension/finder/src/Extension/Finder.php [62]

    -public function onExtensionAfterInstall(AbstractExtensionEvent $event): void
    +public function onExtensionAfterInstall(AfterInstallEvent $event): void
     
    Suggestion importance[1-10]: 10

    Why: The suggestion correctly identifies a mismatch in the parameter type for the method onExtensionAfterInstall, which should align with the dispatched event type for consistency and correctness.

    10
    Add a null check for fileCreator before calling its create method

    In the onExtensionAfterInstall method, the create method of fileCreator should be called
    only if the fileCreator is not null to avoid potential null pointer exceptions.

    plugins/extension/namespacemap/src/Extension/NamespaceMap.php [84-86]

    -if ($event->getEid()) {
    +if ($event->getEid() && $this->fileCreator !== null) {
         // Update / Create new map
         $this->fileCreator->create();
     }
     
    Suggestion importance[1-10]: 7

    Why: This is a good suggestion to prevent potential null pointer exceptions by ensuring fileCreator is not null before invoking its method, enhancing the robustness of the code.

    7
    Add a null check for installer before accessing its properties in the onExtensionAfterUninstall method

    In the onExtensionAfterUninstall method, add a null check for installer before accessing
    its properties to prevent potential null pointer exceptions.

    plugins/extension/joomla/src/Extension/Joomla.php [195-196]

    -if ($eid && $removed) {
    +if ($eid && $removed && $this->installer !== null) {
         $this->removeUpdateSites($eid);
     }
     
    Suggestion importance[1-10]: 7

    Why: Adding a null check for installer is a prudent measure to avoid null pointer exceptions, especially since the method relies on properties of installer. This suggestion improves the safety and reliability of the method.

    7
    Possible issue
    Remove the redundant event dispatch call to avoid triggering the same event twice

    The @TODO comment indicates that the event is dispatched twice in the cleanUp method. To
    avoid redundancy, remove the first dispatch call.

    administrator/components/com_joomlaupdate/src/Model/UpdateModel.php [887-888]

    -// @TODO: The event dispatched twice, here and at the end of current method. One of it should be removed.
    -$app->getDispatcher()->dispatch('onJoomlaAfterUpdate', new AfterJoomlaUpdateEvent('onJoomlaAfterUpdate'));
    +// Trigger event after joomla update.
    +$app->getDispatcher()->dispatch('onJoomlaAfterUpdate', new AfterJoomlaUpdateEvent('onJoomlaAfterUpdate', [
    +    'oldVersion' => $oldVersion ?: '',
    +]));
     
    Suggestion importance[1-10]: 8

    Why: The suggestion is valid as it addresses a redundancy issue where an event is dispatched twice in the same method, which could lead to unintended behaviors or performance issues.

    8
    Maintainability
    Use class constants for event names to avoid potential issues with method name changes

    To avoid potential issues with method name changes, consider using class constants for the
    event names in the getSubscribedEvents method.

    plugins/installer/override/src/Extension/Override.php [51-58]

     return [
    -    'onExtensionBeforeUpdate'    => 'onExtensionBeforeUpdate',
    -    'onExtensionAfterUpdate'     => 'onExtensionAfterUpdate',
    -    'onJoomlaBeforeUpdate'       => 'onJoomlaBeforeUpdate',
    -    'onJoomlaAfterUpdate'        => 'onJoomlaAfterUpdate',
    -    'onInstallerBeforeInstaller' => 'onInstallerBeforeInstaller',
    -    'onInstallerAfterInstaller'  => 'onInstallerAfterInstaller',
    +    self::EVENT_EXTENSION_BEFORE_UPDATE    => 'onExtensionBeforeUpdate',
    +    self::EVENT_EXTENSION_AFTER_UPDATE     => 'onExtensionAfterUpdate',
    +    self::EVENT_JOOMLA_BEFORE_UPDATE       => 'onJoomlaBeforeUpdate',
    +    self::EVENT_JOOMLA_AFTER_UPDATE        => 'onJoomlaAfterUpdate',
    +    self::EVENT_INSTALLER_BEFORE_INSTALLER => 'onInstallerBeforeInstaller',
    +    self::EVENT_INSTALLER_AFTER_INSTALLER  => 'onInstallerAfterInstaller',
     ];
     
    Suggestion importance[1-10]: 7

    Why: Using class constants for event names is a good practice for maintainability and avoids hard-coding strings, which can lead to errors if event names change.

    7
    Enhancement
    Add a constructor to the class to initialize properties or perform necessary setup

    Add a constructor to the BeforeJoomlaUpdateEvent class to initialize any necessary
    properties or perform any setup required for the event.

    libraries/src/Event/Extension/BeforeJoomlaUpdateEvent.php [21-23]

     class BeforeJoomlaUpdateEvent extends AbstractJoomlaUpdateEvent
     {
    +    public function __construct()
    +    {
    +        // Initialize properties or perform setup
    +        parent::__construct();
    +    }
     }
     
    Suggestion importance[1-10]: 5

    Why: Adding a constructor might be beneficial for initializing properties or performing setup, but it's not strictly necessary unless there's specific initialization required, which isn't indicated in the PR or the suggestion.

    5

    @exlemor
    Copy link

    exlemor commented Aug 24, 2024

    I have tested this item ✅ successfully on 2461713

    Installed multiple extensions (and updated 1 of them and uninstalled them - just in case) successfully..


    This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43617.

    @crommie
    Copy link

    crommie commented Aug 24, 2024

    I have tested this item ✅ successfully on 2461713

    Installation extensions still works after applying patch. Site also still works.


    This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43617.

    @richard67
    Copy link
    Member

    @exlemor @crommie Have you also tested that Joomla Updates work as before? The testing instructions tell to do so. It could be done e.g. by applying the patch to a 5.2-dev and then updating to the patched package created by drone.

    @crommie
    Copy link

    crommie commented Aug 24, 2024

    🙈 no. Can I do that on a PBF server site?

    @richard67
    Copy link
    Member

    🙈 no. Can I do that on a PBF server site?

    @crommie I think yes. They can later revert it to the initial state, as far as I know.

    @degobbis
    Copy link
    Contributor

    I have not tested this item.

    Install and uninstall of Extension works as expected.


    This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43617.

    @degobbis
    Copy link
    Contributor

    I have tested this item ✅ successfully on 2461713

    Install and uninstall of Extension works as expected.


    This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43617.

    @crommie
    Copy link

    crommie commented Aug 24, 2024

    🙈 no. Can I do that on a PBF server site?

    @crommie I think yes. They can later revert it to the initial state, as far as I know.

    Just checking: does "reinstall core files" use the same process or is that entirely different?

    @richard67
    Copy link
    Member

    richard67 commented Aug 24, 2024

    🙈 no. Can I do that on a PBF server site?

    @crommie I think yes. They can later revert it to the initial state, as far as I know.

    Just checking: does "reinstall core files" use the same process or is that entirely different?

    @crommie Reinstall core is ok, that also uses the Joomla Update component as if it was an update.

    P.S.: But you should be on the custom update URL created by Drone for this PR so the PR is still applied after the update.

    @exlemor
    Copy link

    exlemor commented Aug 24, 2024

    Hi @richard67,

    Based on your comment, I tested the Joomla Update process for this PR and it worked :)

    Thanks.

    @richard67
    Copy link
    Member

    @exlemor Could you submit again your test result? @obuisard has updated the branch but not restored the previous human tests in the issue tracker, so the test count got reset.

    @exlemor
    Copy link

    exlemor commented Aug 24, 2024

    Sure @richard67 , I will submit it again.

    Olivier just told me he didn't have rights to restore the human tests. ;(

    @exlemor
    Copy link

    exlemor commented Aug 24, 2024

    I have tested this item ✅ successfully on dcf15b1

    I have tested this item successfully

    Installed multiple extensions (and updated 1 of them and uninstalled them - just in case) successfully..
    I also tested the Joomla Update process.


    This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43617.

    @degobbis
    Copy link
    Contributor

    I have tested this item ✅ successfully on dcf15b1


    This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43617.

    @richard67
    Copy link
    Member

    RTC


    This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43617.

    @joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 24, 2024
    @Hackwar Hackwar enabled auto-merge (squash) August 28, 2024 12:57
    @Hackwar
    Copy link
    Member

    Hackwar commented Aug 28, 2024

    Thank you for this contribution @Fedik.

    @Hackwar Hackwar added this to the Joomla! 5.2.0 milestone Aug 28, 2024
    @Hackwar Hackwar merged commit 153b66d into joomla:5.2-dev Aug 28, 2024
    3 checks passed
    @joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 28, 2024
    @Fedik Fedik deleted the plg-event-extension branch August 28, 2024 18:45
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    9 participants