Problem/Motivation

The NodeType plugin makes calls to the deprecated node_type_get_types().

Proposed resolution

Change it to use EntityManager->getStorage('node_type')->loadMultiple()

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#15 2281325-nodetype-injection-15.patch3.21 KBkim.pepper
#13 2281325-nodetype-injection-13.patch3.22 KBkim.pepper
#10 2281325-nodetype-injection-10.patch3.34 KBkim.pepper
#8 2281325-nodetype-injection-8.patch2.9 KBkim.pepper
#4 interdiff.txt3.03 KBkim.pepper
#3 2281325-nodetype-injection-3.patch3.18 KBkim.pepper
#2 2281325-nodetype-injection-2.patch3.22 KBkim.pepper

Comments

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Issue summary: View changes
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Active » Needs review
StatusFileSize
new 2281325-nodetype-injection-2.patch3.22 KB

This adds in dependency injected EntityManager.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
StatusFileSize
new 2281325-nodetype-injection-3.patch3.18 KB

Discussed with @tim.plunkett at the austin sprint, and changed from injecting entity manager, and using entity storage directly instead.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
StatusFileSize
new interdiff.txt3.03 KB

Forgot the interdiff

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 2281325-nodetype-injection-3.patch, failed testing.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Issue tags: +Needs reroll
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2281325-nodetype-injection-8.patch2.9 KB

Re-roll.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Looks good, just two doc nitpicks.

  1. +++ b/core/modules/node/src/Plugin/Condition/NodeType.php
@@ -22,7 +25,37 @@
+   * The entity manager.
+   * @var \Drupal\Core\Entity\EntityManagerInterface

    Missing blank line 

  2. +++ b/core/modules/node/src/Plugin/Condition/NodeType.php
@@ -22,7 +25,37 @@
+   * {@inheritdoc}

    Hmm, I don't think we're allowed to do that with {@inheritdoc}.

    I'd just copy the docs from ContainerFactoryPluginInterface

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
StatusFileSize
new 2281325-nodetype-injection-10.patch3.34 KB

I missed the changes in #3 in the last reroll. Damn you patched based workflow!!

Here's a re-roll of #3 with the fixes for #9.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Looks good!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2281325-nodetype-injection-10.patch, failed testing.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review
StatusFileSize
new 2281325-nodetype-injection-13.patch3.22 KB

Re-roll.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Thanks

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
StatusFileSize
new 2281325-nodetype-injection-15.patch3.21 KB

Re-roll.

alexpott’s picture

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

Committed ffdefc6 and pushed to 8.x. Thanks!

  • alexpott committed ffdefc6 on 8.x 
    Issue #2281325 by kim.pepper: NodeType condition should use...

Status: Fixed » Closed (fixed)

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