Upgrade path from D9 (php 7.4 compatibility)

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! 🙏

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.

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.

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:
📌 Automated Drupal 11 compatibility fixes for admin_toolbar Active
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 😅

OK, so, indeed, we probably have two choices at this point:
1 - Revert the corresponding change so we still support version 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 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.

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.

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.

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 (?!)

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.

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!

Thanks a lot @megakeegman for the MR!

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

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!

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 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.

Adding the other error for completeness

Thanks @megakeegman!

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

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!

Affected version: 3.6.0.