Replace all instances of node_type_load(), node_type_get_types(), entity_load('node_type') and entity_load_multiple('node_type') with static method calls

To test this, I grepped the codebase for the following patterns, before and after applying the patch.

1. node_type_load(
Before patch: 6 instances
After patch: 2 instances (just the function definition and its corresponding doc block)
Status: Resolved

2. node_type_get_types(
Before: patch 10 instances
After patch: 10 instances
Action taken: I have replaced these instances with \Drupal\node\Entity\NodeType::loadMultiple(); with the exception of two places:
- /core/scripts/generate-d7-content.sh (used for D7 sites, not a candidate for ungrading)
- node.module (the original function definition)

3. entity_load('node_type
Before patch: 31 instances
After patch: 0 instance
Status: Resolved

4. entity_load_multiple('node_type
Before patch: 1 instance
After patch: 1 instance
Action Taken: I have replaced this with \Drupal::entityManager()->getStorage('node_type')->loadMultiple()

Updated patch and interdiff attached.

@MKorostoff your patch applies cleanly at commit 093ea79.

I made a reroll and works fine for me :), i have some problems with the interdiff, if someone can give me a hand, I would appreciate it.

I'm starting work on a reroll of the patch, because it doesn't apply anymore.

Rerolled

I will review this starting right now

The patch in #9 doesn't apply cleanly because it contains irrelevant changes.

1. +++ b/core/modules/config/src/Tests/ConfigImportRecreateTest.php
   @@ -98,7 +99,7 @@ public function testRecreateEntity() {
   diff --git a/core/modules/dblog/src/Controller/DbLogController.php b/core/modules/dblog/src/Controller/DbLogController.php
   
   diff --git a/core/modules/dblog/src/Controller/DbLogController.php b/core/modules/dblog/src/Controller/DbLogController.php
   index a11fcb8..89e5dc2 100644
   
   index a11fcb8..89e5dc2 100644
   --- a/core/modules/dblog/src/Controller/DbLogController.php
   
   --- a/core/modules/dblog/src/Controller/DbLogController.php
   +++ b/core/modules/dblog/src/Controller/DbLogController.php
   
   +++ b/core/modules/dblog/src/Controller/DbLogController.php
   +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -140,7 +140,7 @@ public function overview() {
   
   @@ -140,7 +140,7 @@ public function overview() {
          $this->t('Message'),
          array(
            'data' => $this->t('User'),
   -        'field' => 'u.name',
   +        'field' => 'ufd.name',
            'class' => array(RESPONSIVE_PRIORITY_MEDIUM)),
          array(
            'data' => $this->t('Operations'),
   @@ -160,6 +160,7 @@ public function overview() {
   
   @@ -160,6 +160,7 @@ public function overview() {
          'variables',
          'link',
        ));
   +    $query->leftJoin('users_field_data', 'ufd', 'w.uid = ufd.uid');
    
        if (!empty($filter['where'])) {
          $query->where($filter['where'], $filter['args']);
   diff --git a/core/modules/dblog/src/Tests/DbLogTest.php b/core/modules/dblog/src/Tests/DbLogTest.php
   
   diff --git a/core/modules/dblog/src/Tests/DbLogTest.php b/core/modules/dblog/src/Tests/DbLogTest.php
   index 4dc37e4..2ef242d 100644
   
   index 4dc37e4..2ef242d 100644
   --- a/core/modules/dblog/src/Tests/DbLogTest.php
   
   --- a/core/modules/dblog/src/Tests/DbLogTest.php
   +++ b/core/modules/dblog/src/Tests/DbLogTest.php
   
   +++ b/core/modules/dblog/src/Tests/DbLogTest.php
   +++ b/core/modules/dblog/src/Tests/DbLogTest.php
   @@ -65,6 +65,14 @@ function testDbLog() {
   
   @@ -65,6 +65,14 @@ function testDbLog() {
        $this->verifyEvents();
        $this->verifyReports();
        $this->verifyBreadcrumbs();
   +    // Verify the overview table sorting.
   +    $orders = array('Date', 'Type', 'User');
   +    $sorts = array('asc', 'desc');
   +    foreach ($orders as $order) {
   +      foreach ($sorts as $sort) {
   +        $this->verifySort($sort, $order);
   +      }
   +    }
    
        // Login the regular user.
        $this->drupalLogin($this->any_user);
   @@ -218,6 +226,20 @@ private function verifyEvents() {
   
   @@ -218,6 +226,20 @@ private function verifyEvents() {
      }
    
      /**
   +   * Verifies the sorting functionality of the database logging reports table.
   +   *
   +   * @param string $sort
   +   *   The sort direction.
   +   * @param string $order
   +   *   The order by which the table should be sorted.
   +   */
   +  public function verifySort($sort = 'asc', $order = 'Date') {
   +    $this->drupalGet('admin/reports/dblog', array('query' => array('sort' => $sort, 'order' => $order)));
   +    $this->assertResponse(200);
   +    $this->assertText(t('Recent log messages'), 'DBLog report was displayed correctly and sorting went fine.');
   +  }
   +
   +  /**
       * Generates and then verifies some user events.
       */
      private function doUser() {
   

   Reversed (or previously applied) patch detected!

2. +++ b/core/modules/file/src/Tests/FilePrivateTest.php
   @@ -25,7 +27,7 @@ class FilePrivateTest extends FileFieldTestBase {
   diff --git a/core/modules/filter/css/filter.admin.css b/core/modules/filter/css/filter.admin.css
   
   diff --git a/core/modules/filter/css/filter.admin.css b/core/modules/filter/css/filter.admin.css
   index 753dd76..b67a86a 100644
   
   index 753dd76..b67a86a 100644
   --- a/core/modules/filter/css/filter.admin.css
   
   --- a/core/modules/filter/css/filter.admin.css
   +++ b/core/modules/filter/css/filter.admin.css
   
   +++ b/core/modules/filter/css/filter.admin.css
   +++ b/core/modules/filter/css/filter.admin.css
   @@ -38,12 +38,25 @@
   
   @@ -38,12 +38,25 @@
      margin: 0;
    }
    .filter-help a {
   -  background: transparent url(../../../misc/help.png) right center no-repeat; /* LTR */
   -  padding: 0 20px 0 0; /* LTR */
   +  position: relative;
   +  margin: 0 20px 0 0; /* LTR */
    }
    [dir="rtl"] .filter-help a {
   -  background-position: left center;
   -  padding: 0 0 0 20px;
   +  margin: 0 0 0 20px;
   +}
   +.filter-help a:after {
   +  position: absolute;
   +  top: 0;
   +  right: -20px; /* LTR */
   +  content: '';
   +  display: block;
   +  width: 16px;
   +  height: 16px;
   +  background: transparent url(../../../misc/help.png);
   +}
   +[dir="rtl"] .filter-help a:after {
   +  right: auto;
   +  left: -20px;
    }
    
    .text-format-wrapper .description {
   

   Reversed (or previously applied) patch detected!

3. +++ b/core/modules/tracker/src/Tests/TrackerNodeAccessTest.php
   @@ -28,7 +29,7 @@ protected function setUp() {
   diff --git a/core/themes/seven/css/layout/layout.css b/core/themes/seven/css/layout/layout.css
   
   diff --git a/core/themes/seven/css/layout/layout.css b/core/themes/seven/css/layout/layout.css
   index 7bab284..71fc92b 100644
   
   index 7bab284..71fc92b 100644
   --- a/core/themes/seven/css/layout/layout.css
   
   --- a/core/themes/seven/css/layout/layout.css
   +++ b/core/themes/seven/css/layout/layout.css
   
   +++ b/core/themes/seven/css/layout/layout.css
   +++ b/core/themes/seven/css/layout/layout.css
   @@ -39,6 +39,6 @@
   
   @@ -39,6 +39,6 @@
    /**
     * Add spacing to bottom of pages
     */
   -.region-content {
   +#content {
      margin-bottom: 80px;
    }
    
   

   Reversed (or previously applied) patch detected!

Reroll of the patchn without the unneeded extra's.
I mistakenly compared with a branch that wasn't up to date.

The patch applies cleanly now and looks good to me. Marking it RTBC expecting all tests to pass.

Looking good - there is some unrelated change that has crept into the patch and a couple of plugins that should have the node type storage injected rather than just using the static method.

1. +++ b/core/modules/field_ui/src/Tests/EntityDisplayTest.php
   @@ -7,8 +7,10 @@
   +
   

   Unrelated change

2. +++ b/core/modules/node/src/Entity/Node.php
   @@ -490,6 +490,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +
   

   Unrelated change

3. +++ b/core/modules/node/src/NodeForm.php
   @@ -15,6 +15,7 @@
   +use Drupal\node\Entity\NodeType;
   
   @@ -57,7 +58,6 @@ public static function create(ContainerInterface $container) {
   -
   

   Unrelated change

4. +++ b/core/modules/node/src/Plugin/views/argument/Type.php
   @@ -9,6 +9,7 @@
   +use Drupal\node\Entity\NodeType;
   
   @@ -34,7 +35,7 @@ function title() {
   -    $type = entity_load('node_type', $type_name);
   +    $type = NodeType::load($type_name);
   

   Should be injected

5. +++ b/core/modules/node/src/Plugin/views/field/Type.php
   @@ -10,6 +10,7 @@
   +use Drupal\node\Entity\NodeType;
   
   @@ -46,7 +47,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -      $type = entity_load('node_type', $data);
   +      $type = NodeType::load($data);
   

   Should be injected

Hi alex, could you elaborate on that DI vs static method issue ?
I guess i'm just unclear about when DI is the way to go and when it isn't.
Should we always be using DI in a class context ?
If you could point me to some documentation, that would be good as well :)
All i found was https://www.drupal.org/node/2133171, but i guess i'm missing the 'best practices' side of it all ...

I have rerolled the patch and then fixed for comments in #16. The interdiff is only for the fixes for comments and not the reroll.

I hope I got the DI part right.

Wow, that was a silly error. Attaching the interdiff and patch.

I think that we can fix some of the method names to fit the naming convention. Does it make sense to do it here or a new issue?

From other patches i have gathered that mixing in new changes into existing patches is generally not a good idea.

As for the patch (which i was planning to do today since i just figured out what to do ... seems you beat me to it :))
Your dependency injection looks like what i wanted to do too, just a small remark/question:

+++ b/core/modules/node/src/Plugin/views/argument/Type.php
@@ -18,6 +20,39 @@
+  /**
+   * Constructs a Drupal\Component\Plugin\PluginBase object.
+   *

+++ b/core/modules/node/src/Plugin/views/field/Type.php
@@ -20,6 +21,42 @@
+  /**
+   * Constructs a Drupal\Component\Plugin\PluginBase object.
+   *

Shouldn't these be more specific ? It's actually creating a Type plugin object (by first calling the PluginBase construct method but then adding the storage to it)
It seems that throughout the Drupal code, the documentation is not very consistent on this subject.

@oenie: I hope you don't mind. It was 3 days old and I just went for it.

You are right, it is generally not a good idea to mix various issues, but I am not sure if changing the method name had another issue, and if this is the only instance (it is almost like a private method) it doesn't follow the convention, we could consider just fixing it here. I would say it is important as this is Drupal core and even if this method is not extended, there are good chances that someone will use this as reference for writing their own modules and it wouldn't look good.

About the comment, you are right again. I just copied the constructor from another argument (Vid.php, I think). I will have to do some more looking around to see if there is a reason we mention the base object here.

Not at all. I was just waiting for input from alexpott, but it turns out i was looking at the wrong class for the DI.
Things need to move on ! And i just finished another patch with DI, so i got around to understanding it there ...

I copied stuff too, and that's how things like this propagate. I was thinking however that, when you can write 'inheritdoc' (for the create method), it's actually doing the same thing. It's copying the docs from the parent and thus putting something like 'Creates a something something base class'.

Consistency is something to strive for, just no sure how important it is in this case.

That's true. I agree about the consistency. Very important.

If you are thinking about inheritdoc, we can't put it on the constructor because that particular constructor does not have the same parameters as the parent. There is an additional parameter (the one we injected) and we have to document that. As for create, the parameters are same and it's there. :)

I was actually referring to the fact that by putting 'inheritdoc' above the create method, you are (as the name says it) inheriting the parent documentation.
Which would mean, if you generate the documentation, that for the derived class, there would a be a line that says something like 'Creating an object of type
', instead of 'Creating an object of type ', which is what i was trying to avoid in #22. Which would be kinda rendering our consistency ... well ... inconsistent :)

Re-rolled. Also took into account #22

Moving this to the right component :)

#31 apply successfully

The patch does not apply. I am working on the reroll.

Rerolled.

Edit: .... I posted a reverse interdiff. Pardon.

BulkFormAccessTest.php cropped up with entity load.

node_access_test_add_field(entity_load('node_type', 'article'));

Reroll following #2345833: Convert assetEqual to assertIdentical in migrate_drupal

Another reroll for #2357199: Consider CommentManagerInterface::addDefaultField() as deprecated and remove in favour of CommentTestTrait

Applied patch in 41 against cab6a8c22d11ef93637d7806cd62591ec1a2f66a without problems, all good.

Reroll for #2413759: Move D6 dumps to avoid collisions with D7 dumps

1. /core/scripts/generate-d7-content.sh line 97:

   $node_types = $i > 11 ? array('page') : array_keys(node_type_get_types());
   

   Still one call to node_type_get_types in generate-d7-content.sh but I think we don't have to worry about this one?

2. /core/modules/node/node.module line 243:

    * @see node_type_load()
   

   Still an occurrence of node_type_load. Should be replaced with NodeType::load() ?

3. +++ b/core/modules/node/src/Plugin/views/argument/Type.php
   @@ -18,6 +20,39 @@
   +    $this->nodeTypeStorage = $storage;
   

   Rename this to $node_type_storage for clarity?

4. +++ b/core/modules/node/src/Plugin/views/field/Type.php
   @@ -20,6 +21,42 @@
   +    $this->nodeTypeStorage = $storage;
   

   Rename this to $node_type_storage for clarity?

5. +++ b/core/modules/node/src/Tests/NodeCreationTest.php
   @@ -171,7 +173,7 @@ function testNodeAddWithoutContentTypes () {
   +    foreach (\Drupal::entityManager()->getStorage('node_type')->loadMultiple() as $entity ) {
   

   Why are we using \Drupal::entityManager()->getStorage('node_type')->loadMultiple() here and NodeType::load() earlier in this file?

6. +++ b/core/modules/system/entity.api.php
   @@ -1902,7 +1902,7 @@ function hook_entity_extra_field_info() {
   +  foreach (\Drupal\node\Entity\NodeType::loadMultiple() as $bundle) {
   

   Add use statement above and just use NodeType::loadMultiple() here.

7. +++ b/core/modules/system/entity.api.php
   @@ -1940,7 +1940,7 @@ function hook_entity_extra_field_info() {
   -  foreach (node_type_get_types() as $bundle) {
   +  foreach (\Drupal\node\Entity\NodeType::loadMultiple() as $bundle) {
   

   Add use statement above and just use NodeType::loadMultiple() here.

Also, a couple of nitpicks

+++ b/core/modules/node/src/Plugin/views/argument/Type.php
@@ -18,6 +20,39 @@
+   * Database Service Object.

This is not a Database service object

+++ b/core/modules/node/src/Tests/NodeCreationTest.php
@@ -9,6 +9,8 @@
+use Drupal\node\Entity\Node;

This is unused

Addressing comments in #44 and #45. For #44.6 and #44.7, I was going to say it was okay as this is just a reference file and is probably a good idea to show the entire path (as only that section will come up in doc pages). But then I saw that there were other use statements and added accordingly.

Rerolled.

Removing Needs reroll tag.

1. +++ b/core/modules/node/src/Plugin/views/argument/Type.php
   @@ -18,6 +20,39 @@
   +    return new static($configuration, $plugin_id, $plugin_definition, $entity_manager->getStorage('node_type'));
   

   Lets move this onto multiple lines.

2. +++ b/core/modules/node/src/Plugin/views/field/Type.php
   @@ -20,6 +21,42 @@
   +    return new static($configuration, $plugin_id, $plugin_definition, $entity_manager->getStorage('node_type'));
   

   Same for this.

Other than that, looking good.

@benjy: Is this what you meant? (interdiff attached)

No i meant:

return new static(
  $configuration,
  $plugin_id,
  $plugin_definition,
  $entity_manager->getStorage('node_type')
);

Got it :)

Oops! Made a mistake in #54. The interdiff is still against it for continuity.

Thanks, this looks good to me.

This issue was committed (live at DrupalSouth, might I add). I think this should be fixed.

@webchick: You always thank people for contributing, but let me thank you for showing the entire process to everyone in the room. It was a great experience!