Warn site owners about the removal of Admin Toolbar Links Access Filter

Created on 13 November 2024, 6 months ago

Problem/Motivation

Issue #3463291 introduced an update hook that disabled the Admin Toolbar Links Access Filter module when the Drupal core version is 10.3 or above. For site admins that update to this module before updating to Drupal core 10.3 the update is not performed. When in the future the module is removed from the repository this will lead to fatal errors.

Proposed resolution

Inform site admins on the status report.

User interface changes

An error message will be on /admin/reports/status.

I will provide a patch to add the hook_requirements information.

🐛 Bug report
Status

Active

Version

3.5

Component

Code

Created by

🇳🇱Netherlands ericmulder1980

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @ericmulder1980
  • Merge request !103Update admin_toolbar.install → (Closed) created by ericmulder1980
  • 🇳🇱Netherlands ericmulder1980

    Please review the merge request.

  • Pipeline finished with Failed
    6 months ago
    Total: 367s
    #337621
  • 🇳🇱Netherlands ericmulder1980

    For people needing a patch file, see attached.

  • Pipeline finished with Success
    6 months ago
    Total: 374s
    #337638
  • 🇳🇱Netherlands ericmulder1980

    Fixed some coding standards.

    Also i have a question about the version the hook_update is targeting. Issue #3463291 📌 Deprecate the admin_toolbar_links_access_filter module Active mentions the Admin Toolbar Links Access Filter module is obsolute since the Drupal core issue has been fixed in version 10.2. Then, why do we target the update for 10.3 and above?

  • Status changed to Needs review 4 months ago
  • 🇵🇰Pakistan dewancodes

    @ericmulder1980 When we can expect the final MR to be accepted? I want to upgrade this module.

  • First commit to issue fork.
  • Pipeline finished with Failed
    4 months ago
    Total: 516s
    #412537
  • First commit to issue fork.
  • 🇳🇱Netherlands ericmulder1980

    @dewancodes i can't accept my own merge request. It needs to be reviewed by the community and the module maintainer then chooses to merge this into develop.

  • Pipeline finished with Failed
    2 months ago
    Total: 486s
    #448810
  • Pipeline finished with Success
    2 months ago
    Total: 488s
    #448818
  • 🇫🇷France dydave

    dydave changed the visibility of the branch 3.x to hidden.

  • 🇫🇷France dydave

    Thanks everyone!

    I've taken a quick look at this issue and checked the changes from MR!103, which seem to work as expected and display an error message on the status report page, see:

    But since the module was marked deprecated in 📌 Deprecate the admin_toolbar_links_access_filter module Active , there is already a warning message appearing on the status report page, see:

    Additionally, there is a clear warning when the module is installed through the Admin UI, see:


     

    Do you think these warnings from core and/or other contrib modules are not enough or should be made even more visible?

    Personally, I'm not completely convinced this change would be necessary, but if you think it should be added and "could" help some users, I wouldn't be opposed, given the minimal impacts the corresponding code changes would have on the module.

    I just think the current warning messages are clear enough with helpful links.
    It should be part of site administrator's tasks to check regularly the status report page when proceeding to upgrades.
     

    In case you still think we should move forward with this change, I created a new merge request MR!122 based on the previous one with minor changes:

    • Moved the code to admin_toolbar_links_access_filter/admin_toolbar_links_access_filter.install, instead of admin_toolbar.install, since we'd like that kind of cruft to go away when the module is removed.
    • Updated the wording.
    • Updated the lifecycle (deprecated) URL to point to 📌 Deprecate the admin_toolbar_links_access_filter module Active .

     

    If this issue gets enough traction and more feedback on the merge request, we would be happy to consider adding this to the module.

    Thanks in advance!

  • 🇳🇱Netherlands ericmulder1980

    @dydave,

    Thank you so much for looking into this. I agree that there are already a few warnings in place and that it is the site maintainers responsibility to check these. For many contributed modules this doesn't have a fatal impact as removing them as a whole from the codebase is also a decision often made by or in collaboration with a site maintainer.

    As this is a submodule of a very popular contrib module i do feel that this is something that could be overlooked. As you mentioned the impact on the codebase is minimal and it could prevent fatal errors for lots of users.

    Not sure how much response this will get as most people will only go to the issue queue once they indeed have encountered an issue. Let's see what happens.

  • 🇫🇷France dydave

    Thanks a lot Eric (@ericmulder1980) for your kind, constructive reply and for taking the time to share your thoughts, it's greatly appreciated!

    Thanks a lot for raising this issue and anticipating on any future potential impacts on users projects.

    As mentioned before, I'm favorable to getting this in the module, no problem at all 👌
    If you'd like, we could get this included in the next stable release.

    Let's get the patch validated (RTBC-ed if possible)?
    Were you able to test the patch from the merge request above MR!103, check the wording, check the message and feature (display of the error message)?

    Anything that could help validating and confirming the changes so they could be pending merge for the next stable ...
    We'll then leave a bit of time to get more feedback and before creating the release we'll get this one merged in.

    Again, any help, comments, feedback, questions or suggestions would be greatly appreciated!
    Thanks again Eric and in advance!

  • 🇩🇰Denmark ressa Copenhagen

    Thanks for working on this, just turning the issue reference into a link :)

  • 🇩🇰Denmark ressa Copenhagen

    A release is planned soon (21 May), so this issue needs a review fairly soon, for it to be included ...

  • 🇫🇷France dydave

    Thanks a lot @ressa!

    Worst case: we don't necessarily need to wait for Eric's (@ericmulder1980) feedback:

    Mostly, what remains for this issue:

    Let's get the patch validated (RTBC-ed if possible)?
    Were you able to test the patch from the merge request above MR!122, check the wording, check the message and feature (display of the error message)?

    Anything that could help validating and confirming the changes so they could be pending merge for the next stable ...

    Basically, if you could test the patch:
    On a Drupal Core version above 10.3, apply the patch, then enable the admin_toolbar_links_access_filter module.
    Just check the message displays as expected and whether you think the wording is clear enough....

    That should be enough for us to RTBC the issue and get it merged quickly after 👌

    I wish Eric (@ericmulder1980) could have replied on this, but if we can't get a reply before the next release, then it would be great if you could maybe take a very quick look at this one, just to confirm everything is in order 🙂

    The impact of this MR is very limited, so it should be very safe to merge 👍
    Bear in mind all of this should be trashed once the admin_toolbar_links_access_filter module can be removed from the code base (when support below 10.3 is dropped).

    Let us know if you catch anything or have more suggestions or comments on any aspects of this ticket or the merge request, it would definitely be very helpful! (as always @ressa 😉)
    Thanks in advance!

  • 🇩🇰Denmark ressa Copenhagen

    No problem @dydave, I am only glad I can help, and thank you @ericmulder1980 for creating this issue and MR in the first place :)

    I installed Admin Toolbar dev-version in Drupal 10.4.6, applied this issue's patch, installed the Admin Toolbar Links Access Filter module, and got the warnings you shared in comment #13 🐛 Warn site owners about the removal of Admin Toolbar Links Access Filter Active .

    I went to the Status page and got this Error message (HTML copied form that page):

    Status Details


    Errors found

    Admin Toolbar Links Access Filter
    The features of the Admin Toolbar Links Access Filter module are included in Drupal core since version 10.3 and therefore it should be uninstalled.
    Since the module is no longer needed, it will be removed in future releases of Admin Toolbar when support for versions lower than 10.3 is dropped.

    See issues: #3463291 and #3487246 .

    The wording is spot on, and users who read it, have been informed that the module will be removed in the future, when support for versions lower than 10.3 is dropped. So this issue is RTBC.

    And it will be great to eventually be able to remove this module, and continue adding marginal gains in Drupal :)

  • 🇨🇦Canada adriancid Montreal, Canada

    Thanks

Production build 0.71.5 2024