Replace deprecated library core/jquery.once with core/once per D.O. Issue #3158256

Thanks @lhridley, your patch takes care of this test error:

  4x: The core/jquery.once asset library is deprecated in Drupal 9.3.0 and will be removed in Drupal 10.0.0. Use the core/once library instead. See https://www.drupal.org/node/3158256
    4x in AdminToolbarSearchSettingTest::testToolbarSearch from Drupal\Tests\admin_toolbar_search\Functional

It looks like the admin_toolbar_search/js/admin_toolbar_search.js file also needs to be reworked:

  6x: Javascript Deprecation: jQuery.once() is deprecated in Drupal 9.3.0 and will be removed in Drupal 10.0.0. Use the core/once library instead. See https://www.drupal.org/node/3158256
    5x in AdminToolbarToolsSearchTest::testToolbarSearch from Drupal\Tests\admin_toolbar_search\FunctionalJavascript
    1x in AdminToolbarSearchTest::testToolbarSearch from Drupal\Tests\admin_toolbar_search\FunctionalJavascript

Here is a PR with the asked modifications from #8
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/21

Thanks

This made the core_version_requirement change in #3256600: Add Drupal 10 to required drupal version now be wrong, as core/once was added in Drupal 9.2.0.

I'm asking for the other issue to be re-opened, as the needed changes are easier to see over there.

Also, now that this change has been added to the module, this is a major backwards compatibility break (i.e. no longer compatible with Drupal 8.x, 9.0.x and 9.1.x), and as such the module should bump the major version to 4.x

Agree with #13.

@jcnventura fell free to reopen the other issue.

Only maintainers can reopen it @adriancid.
I created a MR for it but I'm not able to reopen it. Btw I also agree we should create anther branch for it.

As I understand it, the Admin Toolbar module cannot be compatible with Drupal 8 and Drupal 10 at the same time. If we update the code too early to be compatible with Drupal 10, we face problems with Drupal 8 and Drupal 9 early versions. To me we should wait until the release of Drupal 10 and have a 4.0 version of Admin Toolbar.
I think we should cancel the change here and wait a few month.

I reverted the commit, so that we don't break BC. I will change this issue to Postponed until we release a 4.x branch for Drupal 10.

Can this be unpostponed yet? Drupal core 9.2 no longer receives support.

I think this can be unpostponed. Considering that https://www.drupal.org/project/admin_toolbar/releases/3.1.1 was just released with supposed support for D10. But without the changes here, the sub module for search won't fully work.

@heddn yes I think that it was an error to add Drupal 10 compatibility as Admin Toolbar is not in fact compatible! I will create an issue to remove that. We should go toward a new 4 version of the module for Drupal 10.

Thanks @romainj, perhaps a "Create 4.x branch for Drupal 10" issue should be created, and list up the tasks required to make it happen?

@ressa Yes we need to create a dedicated issue and a new 4.x branch from the latest 3.x version.

It is mind blowing how much energy contrib maintainers put into supporting unsupported Drupal core releases.
Support for D8 and D9.2 can be dropped in any new release, nobody should complain.

@Pasqualle True. But, the fact is that if you are following semantic versioning, you should increase the major version if you break backwards compatibility. It is up to the individual maintainer if they will want to respect that or not, but for those who do, there's not much difference. Pretty sure that if branch 4.x is created here, once 4.0.0 is released the support for the 3.x versions will stop.

Thanks @romainj, it sounds good. I just got the newsletter from Drupal Association team, reminding me how close Drupal 10 is:

The expected release date of 14 December 2022 for Drupal 10 is coming two months from today, and we could not be more excited to share it with the Drupal community!

Yes D10 is closed! I would like to deal with some issues before cloning the 3.x branch into the 4.x. We will have a D10 compatible version on time :)

Sounds good @romainj! To help out a little, I have created #3315478: Create 3.3.x release for Drupal 10. Feel free to add the blocking issues you mention in that issue, so that me and others can give them attention they need, to get D10 released.

> @Pasqualle True. But, the fact is that if you are following semantic versioning, you should increase the major version if you break backwards compatibility

This has been discussed before, but no, increasing the required version of your dependencies is _not_ a BC break. It's a reason for a minor bump, not a major. See https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-wit.... This is not a public API of this module.

Not if it's defined properly, that is. This was committed too early and without fixing the core compatibility, so the revert made sense.

But my approach as a maintainer would be to commit this and release 3.2, not 4.0. Which has the important advantage that most sites (that bother to update) will automatically update to that version if they use a supported core version. And thanks to semver, you still could do a 3.1.x security release easily. In my experience, you have to expect at least 1y until a considerable amount of users switch over to a new major version and it makes your life and those of your users more annoying. Dozens of contrib modules did the same, including pretty much every single top10 module now requires 9.3 or even 9.4.

@Berdir The latest stable release is 3.2.1. As I understand it we should release the 3.3.0 version with support for d9 and d10. In case of security patch we could release a 3.2.2 (for d8 and d9) and a 3.3.1 versions (for d9 and d10). Am I right

@romainj Yes, ideally you'd start a 3.2.x branch for that, but you can also leave it and take care of that later, as you can always reset to the last commit before the d9/d10 support and then create the branch at that time.

*if* you care about supporting core versions older than 9.3 then yes, my approach would be to release this as a new minor and not a new major version.

The development branch is 3.x, not 3.2,x, so this does not require a new branch. you could branch of from the tag if you want to do a 3.2 (security release).

btw, I noticed that the 3.1 releases have been marked as unsupported which is IMHO a much bigger annoyance than no longer supporting 9.2 and older.

So what is still needed for this ticket to get a D10 release? Is the issue resolved and the discussion around what release it should be in?

Working on Drupal 10 core is slow and cumbersome without Admin Toolbar. So much clicking. Admin Toolbar Quick Search is also severely missed ... It might also stop users from starting new projects directly in Drupal 10.

So it would be super awesome to get a release for Drupal 10 soon, even if it doesn't work 100% out of the box and needs a patch or two.

Based on the comments on this issue the best decision would be to create the following minor version including all things needed for this module to work with D10, I also agree that at this point is not needed for this important module to wait for a major release.

Please @romainj let me know how can I help to make this possible, maybe we need to change the title in #3315478: Create 3.3.x release for Drupal 10 and coordinate all the efforts from that one.

Regarding to this issue I think we can go ahead and add this change, because if someone needs compatibility with versions older than 9.3 then they can just use 3.2.x and will be possible to create security patches for that version.

+1 for the idea above.

I recreated the previous MR which was ok, but now changing also the minimum core require version, so we can proceed with this issue and focus on other D10 compatibility issues.

looks good to me. D9 site doesn't appear broken.

No needs to go back to NW, but according to the core/once change record $(elements).each() can be elements.forEach().
A small step towards dropping jQuery dependency. :D

Just my 2 cents.

that requires a different syntax, with passing in the item and is imho only worth doing if jquery could easily be dropped completely. That would also make this more complicated to review.

I completely agree with @Berdir, I think we need to work on removing jQuery dependency in another issue.

Following @Berdir's suggestion in #3285981: Automated Drupal 10 compatibility fixes to merge the two issues, I'm adding the changes to the core_version_requirement to all the sub-modules, and adding support to Drupal 10. Also removing the redundant drupal/core requirement from composer.json, as that is precisely what the Drupal packagist will overwrite with the information on core_version_requirement.

Only the admin_toolbar_search module is using the jQuery once, so IMHO it is the only one which requires 9.2 as the minimum version after this change.

I agree we can merge both issues because the changes are not too complex.

so IMHO it is the only one which requires 9.2 as the minimum version after this change

Theoretically true, but AFAIK if we don't put the core:^9.2 dependency on the parent module, a D9.1 site would be able to upgrade admin_toolbar to $this version. Then what would happen if admin_toolbar_search is already installed?

I don't know the answer, and can't test myself. So I'm +1 with @jcnventura changes. This issue is already RTBC.

Yes, from a composer point of view, you need to add 9.2 to the root module, or else you'll have >=9.2 code in a 8.9 or 9.1 site install.

And once the root module is only 9.2, then all others need to be 9.2, so I believe this is the only solution, short of forking the sub-module to it's own Drupal project.

Thanks to everyone involved.