Problem/Motivation

Remove all D12 deprecated code from the mentioned libraries.

Steps to reproduce

Proposed resolution

Remove all D12 deprecated code from the mentioned libraries.

Remaining tasks

Make sure tests are passing before review.

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3571057

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

smustgrave created an issue. See original summary.

smustgrave opened merge request !14778

smustgrave’s picture

smustgrave commented

There are some todos in the tests but will be covered by #3468921: Convert trigger_error in VariationCache into a LogicException

smustgrave’s picture

smustgrave commented
Status: Active » Needs review

This one is ready.

dcam’s picture

dcam commented

Can you verify that core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php should be deleted? The test still passes. The mock object in the setUp() was updated to be LibraryDiscoveryCollector instead of LibraryDiscovery. But the CoversClass attribute was not updated. If it should be kept, then maybe we should update the file/class name too.

dcam’s picture

dcam commented

Otherwise this looks good to me. All of the deprecations have been removed.

dcam’s picture

dcam commented
Status: Needs review » Needs work

In addition to the question in #5, I found some other things:

core/lib/Drupal/Core/Cache/ChainedFastBackend.php, core/lib/Drupal/Core/Cache/NullBackend.php, and core/modules/system/tests/modules/performance_test/src/Cache/CacheBackendDecorator.php still have invalidateAll() methods. I don't know whether these are supposed to remain in place or not. But even if they are valid, their docblocks are not. They all have @inheritdoc docblocks, which doesn't work anymore because the methods aren't overriding anything.

invalidateAll() is mentioned in a docblock in line 502 of core/core.api.php.

A grep for the LibraryDiscovery class revealed these issues:

~/drupal_core$ grep -rn "LibraryDiscovery" core
core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php:544:            // @see \Drupal\Core\Asset\LibraryDiscovery::getLibraryByName()
core/core.services.yml:1771:    deprecated: The "%service_id%" service is deprecated in drupal:11.1.0 and is removed from drupal:12.0.0. Use LibraryDiscovery instead. See https://www.drupal.org/node/3462970
smustgrave’s picture

smustgrave commented

Pushed some of the feedback but need to look into the service the message seems wrong.

dcam’s picture

dcam commented

...the message seems wrong.

Right? I didn't get it, but I also didn't look into it.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Okay I think the message meant to say it was replaced by library.discovery. The service above it.

dcam’s picture

dcam commented
Status: Needs review » Needs work

There's an open question in #5.

I mentioned https://git.drupalcode.org/project/drupal/-/blob/main/core/lib/Drupal/Co... in #7, but that grep output was badly formatted. So that's on me.

Thank you for figuring out what was up with that service deprecation message. Your explanation seems likely to me.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Believe it's good to be deleted. In the same folder there is LibraryDiscoveryCollectorTest which appears identical. Think the original test was updated to LibraryDiscoveryCollector was to match the service change.

smustgrave’s picture

smustgrave commented

Rebased

dcam’s picture

dcam commented
Status: Needs review » Needs work

I'm sorry, @smustgrave. I keep saying this badly:
There is a lingering reference to the deleted LibraryDiscovery class in the LibraryDiscoveryParser class. It needs to be fixed. See https://git.drupalcode.org/project/drupal/-/blob/main/core/lib/Drupal/Co....

I apologize for not being 100% clear about it.

smustgrave’s picture

smustgrave commented
Assigned: Unassigned » smustgrave

Will get to it

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Removed that reference, was nervous it was going to complain about 81 characters but looks like we are good.

dcam’s picture

dcam commented
Status: Needs review » Needs work

I'm sorry @smustgrave, but the referenced class name needs to be changed. I left a suggestion.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

Thank you. I feel like all I've done lately is point out nitpicky things for you to change.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

One test being removed that I'm pretty sure needs to stay. Can probably double check by restoring the test and see if things are still green afterwards.

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

I added the test back, I did have to update the classCover attribute since it's not covering LibraryDiscovery

  • catch committed 870a5021 on main 
    task: #3571057 Remove deprecated code from lib/Asset and lib/Cache

By:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Test changes look good. Committed/pushed to main, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !14778

Status: Fixed » Closed (fixed)

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