Problem/Motivation

Remove the entity manager service and related classes

Proposed resolution

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

The deprecated entity manager service and supporting code has been removed.

CommentFileSizeAuthor
#16 3087546-16.patch442.67 KBalexpott
#14 bye-entity-manager-3087546-14.patch443.18 KBberdir
#10 bye-entity-manager-3087546-10-interdiff.txt8.92 KBberdir
#10 bye-entity-manager-3087546-10.patch443.29 KBberdir
#3 bye-entity-manager-3087546-3-interdiff.txt5.09 KBberdir
#3 bye-entity-manager-3087546-3.patch438.77 KBberdir
#2 bye-entity-manager-3087546.patch434.49 KBberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new bye-entity-manager-3087546.patch434.49 KB

Maybe this was a bit much at once :)

This removes entity manager class/service, a ton of constructor updates, and a bunch of deprecated files/classes that depending on it still, including entity.inc, simpletest KernelTestBase and so on.

Also quite a few documentation cleanups of old "entity manager" references.

With this patch, there is no mention of "entity.?manager" or the change record node ID left.

Probably missed a few calls on depending things that I removed, lets see.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: 8.9.x-dev » 9.0.x-dev
StatusFileSize
new bye-entity-manager-3087546-3.patch438.77 KB
new bye-entity-manager-3087546-3-interdiff.txt5.09 KB

Test run seems to be going fairly well, a few fixes.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Things we could do to get the size of this down include for example removing all deprecated test base classes and traits first, including KernelTestBase.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

193 files changed, 182 insertions, 5945 deletions. <3 - how cool is that.

martin107’s picture

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

Wow I am excited to see all that code vanish.

I sat down while fresh and had a visual scan of the patch ... I could justify all the changes.

It is difficult to absolutely prove that ALL the changes have been made
It is the nature of +400k patch that they are borderline visually review-able.

In addition I searched for the obvious candidates

entityManager,
entitiy.manger,
entity_manager,
entity manager

and I can say those words no longer exist in the codebase

Given the inherent difficulties. I take an slightly modified view from bedir.

I say he has clearly identified the bulk of the tasks. And we should view this as a standalone work item. We should commit this as it ...

As we live with the codebase we will see followups and it is fine to create followups.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Needs review

Well, first we'll need to at least make Drupal 9 actually installable, but thanks for the review & RTBC!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Firefox, sometimes you do strange things with selects.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Reviewed & tested by the community » Needs work

There are some PHPCS warnings on the test run.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new bye-entity-manager-3087546-10.patch443.29 KB
new bye-entity-manager-3087546-10-interdiff.txt8.92 KB

Yeah, if only PhpStorm would automatically remove use statements... :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

vendor/bin/phpcbf --standard=./core/phpcs.xml.dist core/ is FTW.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Issue tags: +Deprecation Removal
voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

#10 Needs reroll for 9.0.x branch

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new bye-entity-manager-3087546-14.patch443.18 KB 
error: patch failed: core/modules/file/tests/src/Functional/FileFieldValidateTest.php:197
Falling back to three-way merge...
Applied patch to 'core/modules/file/tests/src/Functional/FileFieldValidateTest.php' with conflicts.
error: patch failed: core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php:2
Falling back to three-way merge...
Applied patch to 'core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php' with conflicts.
U core/modules/file/tests/src/Functional/FileFieldValidateTest.php
U core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php

Both easy to resolve. \Drupal\Tests\file\Functional\FileFieldValidateTest::testAssertFileExistsDeprecation() could possibly be removed completely, but seems a bit too off topic and non-trivial as there's some phpunit compatibility stuff going on and we'd need to remove that trait too.

And ViewsSelection just conflicted on a new trait that was added...

martin107’s picture

martin107 commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

By doing a file diff between 10 and 14

I can see all Bedir says is true ( see 14 )

I also checked that no additional coding standard errors were introduced.

Sorry this has taken so long to get back to RTBC ... I will be dancing around my desk for a week when this gets committed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 3087546-16.patch442.67 KB

Rerolled - there were very simple to resolve conflicts due to #3048498: [≈Nov. 11] Fix Drupal.Commenting.Deprecated coding standard

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
Issue tags: +8.8.0 release notes

Added to https://www.drupal.org/node/2549139 and a release notes snippet.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: -8.8.0 release notes +9.0.0 release notes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed f2469de and pushed to 9.0.x. Thanks!

  • alexpott committed f2469de on 9.0.x 
    Issue #3087546 by Berdir, alexpott, martin107, jibran: Remove deprecated...

Status: Fixed » Closed (fixed)

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