Problem/Motivation

On Drupal versions prior to 10.2, an error prevents database updates and flushing cache. Error is caused by the DeprecationHelper class not existing prior to 10.2, but being used in ToolbarController.

Initial error

ParseError: syntax error, unexpected '=>' (T_DOUBLE_ARROW), expecting ')' in Composer\Autoload\{closure}() (line 198 of /var/www/html/web/modules/contrib/admin_toolbar/admin_toolbar_tools/src/Controller/ToolbarController.php)

Error after fixing syntax issues

Error: Class 'Drupal\Component\Utility\DeprecationHelper' not found in Drupal\admin_toolbar_tools\Controller\ToolbarController->flushJsCss() (line 198 of modules/contrib/admin_toolbar/admin_toolbar_tools/src/Controller/ToolbarController.php)

Issue fork admin_toolbar-3528007

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

megakeegman created an issue. See original summary.

dydave’s picture

dydave commented
Category: Bug report » Task

Thanks @megakeegman for raising this issue. 🙂

Could we potentially requalify this as a Task instead of a Bug report?

Ideally, we would like to keep the bugs count focused on the actual bugs/problems/issues of the releases that are currently supported, in order to give users a more accurate vision of the current level of stability of the module.

Feel free to let us know if you would have any objections, concerns or questions this particular comment or the project in general, we would surely be glad to help.
Thanks in advance for your understanding and help! 🙏

megakeegman’s picture

megakeegman commented

Actually I was just reviewing the issue, the code that threw the error (sorry I don't have the error, was dealing in bulk on Friday) is in the Toolbar Controller.

DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.2.0', fn() => $this->assetQueryString->reset(), fn() => _drupal_flush_css_js());

If I understand correctly, DeprecationHelper class was introduced in Drupal 10.2, correct? If that is the case, maybe support for Drupal 9 should actually be removed? And maybe this issue is broader than my initial description.

megakeegman’s picture

megakeegman commented

I was thinking for my purposes to just add a patch that removes that line, and then remove that patch once I complete the upgrade.

I am happy to follow your guidance on what you think is best for issue queue management here. I'll check in for your feedback later.

dydave’s picture

dydave commented

Thanks @megakeegman for the prompt and very helpful feedback!

If I understand correctly, DeprecationHelper class was introduced in Drupal 10.2,

raaaaa... Great catch! 😅

Looking at the issue that introduced this piece of code again:
#3518123: Fix deprecated call to '_drupal_flush_css_js' for core versions below 10.2
It doesn't make sense really 😅

We're using a method/class introduced in 10.2
https://www.drupal.org/node/3379306
to support versions lower than 10.2???!!

It doesn't make any sense 😅
Not sure how I could have missed that 🙈

OK, so, indeed, we probably have two choices at this point:
1 - Revert the corresponding change so we still support versions lower than 10.2
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/136/di...
which would be the most straight forward, immediate solution, just creating a revert merge request for:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/136

2 - Move forward and drop support for versions lower than 10.2 or 10.3 so we could provide a minor release with a sliding window allowing to upgrade core to 10.3, then further upgrade the module and core to D11, something like that.
So provide support for 10.3 and 11.
This would probably take much more time and involve a lot of code changes, mostly moving on to new APIs, service declarations, removing a lot of BC code, etc....

If option 1 could work for you and you think it could provide an acceptable solution for the time being, then could you please help us create the corresponding revert merge request and maybe try testing it on your setup?
This could definitely be released in an upcoming patch version 3.6.1, whereas dropping a version would probably require a new minor (a bit more complicated/documentation, etc...).

Please let us know if you have any other ideas, suggestions or questions on this particular issue or the module in general, we would surely try answering as soon as possible.
Thanks in advance for your feedback and help.

megakeegman’s picture

megakeegman commented

Oh wow, it occurred to me that that's what was happening, but I just assumed that I didn't understand the code. Anyway, option 1 seems okay, but unfortunately it may not be easy for me to test (at least without spinning up a blank drupal instance). In my attempts to get this current site to the latest version of D9, I have seen so many errors from modules, mostly relating to code updates that are incompatible with php 7.4, hence my assumption that informed the title of this issue. While I have been able to work through the handful of errors that prevent me from updating composer deps and running database updates, I am not really interested right now in fixing all of the other fatal errors that are preventing site usage, since I don't intend to stay on D9. Anyway, I guess the most important takeaway of this message is that it is slowly becoming increasingly difficult to update a Drupal 9 site that has contrib modules installed. I suppose I could try cloning the repo (issue fork) into my project directly (pre-update) for testing purposes.

megakeegman’s picture

megakeegman commented
Title: Upgrade path from D9 (php 7.4 compatibility) » DeprecationHelper class not available before Drupal 10.2
megakeegman’s picture

megakeegman commented
Issue summary: View changes
megakeegman’s picture

megakeegman commented

There was only one merge conflict during the reversion, in phpstan. It was the line 17 addition of a code comment phpstan.neon (at least it was line 17 in https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/136/di...). In later versions, that code was uncommented. I assumed that we wanted to keep that line uncommented, rather than removing it. I'm sure you will say something if this is wrong, but wanted to point it out.

dydave’s picture

dydave commented

Re #6:

Anyway, I guess the most important takeaway of this message is that it is slowly becoming increasingly difficult to update a Drupal 9 site that has contrib modules installed.

Totally agree @megakeegman: This is mostly due to the fact that Gitlab CI does not run build pipelines for 9.x anymore (by default).
In fact, only the current two supported majors are really tested, currently 10 and 11.

Another improvement could be to add the necessary configuration options to the Gitlab CI file of the module to test 9.x as well.
But that would probably be a "nice-to-have" at this point (?!)

dydave’s picture

dydave commented

Re #9:

No problem @megakeegman!
Thanks again very much!

I "think" we're probably going to want to keep the changes in the neon file, but I'm not sure exactly.
If you create a revert merge request, the jobs should run anyway and we should be able to revert or make any other changes to the MR, as required to fix the PHPSTAN errors.

I guess the most difficult part is probably the testing, as you mentioned at #6 and confirming the patch actually fixes the issue.

Feel free to let us know if you have more questions or how we could help, we would be glad to reply as soon as possible.
Thanks in advance!

megakeegman opened merge request !157

megakeegman’s picture

megakeegman commented
Status: Active » Needs review
dydave’s picture

dydave commented

Thanks a lot @megakeegman for the MR!

Were you able to test the changes on your setup?
Did it fix the issue?

Do you see any other piece of code in the module or sub modules that could produce the same kind of issues?
(Compatibility with D9 and the upgrade path)

Otherwise, the tests and jobs are all passing 🟢, so when we could get your confirmation, we should be able to merge the MR ASAP 👌

Thanks in advance!

megakeegman’s picture

megakeegman commented
Status: Needs review » Reviewed & tested by the community

I have been able to test to some extent. As expected, flushing cache does not work without this patch, but it does work when the patch is applied. Also without the patch, I was unable to apply database updates, but the patch also fixes that. I have not experienced any errors related to this while in the Drupal UI (aside from flushing caches), or any other d9 compatibility related errors coming from this module.

I was able to get my D9 site to D10 with the help of this patch

megakeegman’s picture

megakeegman commented
Issue summary: View changes
megakeegman’s picture

megakeegman commented
Issue summary: View changes

I have the initial error handy, so adding it to the description to improve documentation. I had tried resolving the syntax errors I recall, which eventually led me to different errors about the DeprecationHelper class.

megakeegman’s picture

megakeegman commented
Issue summary: View changes

Adding the other error for completeness

dydave’s picture

dydave commented
Status: Reviewed & tested by the community » Needs review

Thanks @megakeegman!

Reviewing and testing locally right now.
This should be merged within the next hour.

  • dydave committed df48f29d on 3.x authored by megakeegman 
    Issue #3528007 by megakeegman, dydave: Reverted change from DO-3518123,...
dydave’s picture

dydave commented
Category: Task » Bug report
Status: Needs review » Fixed

Great job @megakeegman! 🥳

Thanks a lot for your time, patience and great help with all the testing! 🙏

I made a few minor changes to the MR just to keep a few lines from the reverted commit.
I did some testing locally as well and since all the tests and jobs of the merge request were still passing 🟢, I went ahead and merged the changes above at #20 🥳

Great catch, once again and thanks a lot for raising this one so shortly after the release so at least this major problem could be fixed for other users 👍

There are still a couple more issues to go, but this should get released in an upcoming 3.6.1 patch version very soon (I'm hoping next week max / if possible this week 👌).

oh ... btw: requalifying as a bug report 😉
You were right: given the current supported versions, this is definitely a regression, so it should be counted as such in module's stats.

I don't really see any follow-up issue to this: The code from this MR should just get removed (the deprecated function) when support for versions below 10.2 or 10.3 is dropped anyway... so an overall clean-up should be done at that point.

Therefore, marking issue as Fixed for now.

Feel free to let us know if you would encounter any other issues or would have any suggestions or questions on the module, we would surely be glad to help.
Thanks in advance!

dydave’s picture

dydave commented
dydave’s picture

dydave commented
Version: 3.x-dev » 3.6.0

Affected version: 3.6.0.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.