Dependency injection in NodePermissions

Let's trigger the testbot by marking needs review.

Looks good overall, just a nitpick:

+++ b/core/modules/node/src/NodePermissions.php
@@ -32,13 +71,13 @@ public function nodeTypePermissions() {
    * Returns a list of node permissions for a given node type.
    *
-   * @param \Drupal\node\Entity\NodeType $type
+   * @param \Drupal\node\NodeTypeInterface $type
    *   The node type.
    *
    * @return array
    *   An associative array of permission names and descriptions.
    */
-  protected function buildPermissions(NodeType $type) {
+  protected function buildPermissions(NodeTypeInterface $type) {

Those changes seem unrelated to me.

Removing unused Drupal\Core\StringTranslation\TranslationInterface and unused $this->stringTranslation service injection it is not used in file.

Using \Drupal\Core\Entity\EntityStorageInterface instead of Drupal\Core\Config\Entity\ConfigEntityStorageInterface for injecting $this->nodeTypeStorage services.

Fixing some drupal standards error also, Kindly review a new patch.

Removing unused Drupal\Core\StringTranslation\TranslationInterface and unused $this->stringTranslation service injection it is not used in file.

Fixing some drupal standards error also for patch at #7 , Kindly review a new patch.

Will review the patch and provide feedbacks/updates if necessary.

Made compatible to coding standards issue.

@vivek Panicker , kindly upload interdiff also along with the patch whenever you do some changes in patch. It will be helpful for other to track what's exactly changes done.

Rerolled #13 for 9.5

I will revise it.

I'm keep revising it

I revised the reroll #25 I applied in drupal9-9.5 version

Core keeps working as expected.

#13 patch is not applied successfully on 9.5.x , reroll for 9.5.x.

Fixed CCF #25

try to fix

trying Fix the PHPCS error

Fixing CCF errors

Hi I applied the patch #29 after that I ran these phpcs commands :

phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml .

phpcs --standard=DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml .

I found that errors in screenshot

Hi everyone the lasts rerrols erased the changes previous did before. I revised the patch #20 and I was able to apply in drupal 9.5 version, and the changes were kept. You can discard the others patches that was made after patch #20, because it erased the changes of the issue.

As an API addition/internal BC break, this should be targeted for the development branch (currently 10.1.x).

1. +++ b/core/modules/node/src/NodePermissions.php
   @@ -2,25 +2,56 @@
   -use Drupal\Core\StringTranslation\StringTranslationTrait;
    use Drupal\node\Entity\NodeType;
   +use Drupal\Core\StringTranslation\StringTranslationTrait;
   

   This reordering of use statements is out of scope and also against our default pattern of having them alphabetized (so Drupal\Core should come before Drupal\node).

2. +++ b/core/modules/node/src/NodePermissions.php
   @@ -2,25 +2,56 @@
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager) {
   +    $this->nodeTypeStorage = $entity_type_manager->getStorage('node_type');
   

   For BC purposes, since these objects previously would have been constructed directly instead of by the factory method, this should probably allow the entity type manager to default to NULL. The parameter documentation should then be updated accordingly.

   And, if null is passed, it should load the storage conditionally using \Drupal::entityTypeManager(), with a deprecation warning.

3. +++ b/core/modules/node/src/NodePermissions.php
   @@ -2,25 +2,56 @@
   +    return new static(
   +      $container->get('entity_type.manager')
   +    );
   

   This should be on a single line.

4. +++ b/core/modules/node/src/NodePermissions.php
   @@ -2,25 +2,56 @@
      public function nodeTypePermissions() {
   +    // Generate node permissions for all node types.
        return $this->generatePermissions(NodeType::loadMultiple(), [$this, 'buildPermissions']);
   

   This comment addition is out of scope, and also seems unnecessary?

5. +++ b/core/modules/node/src/NodePermissions.php
   @@ -2,25 +2,56 @@
        return $this->generatePermissions(NodeType::loadMultiple(), [$this, 'buildPermissions']);
   

   I think this needs to be changed to use the entity type manager as well? Otherwise, this will load from the default storage even if someone injects a different one, which could lead to data integrity errors.

The issue summary also says this should be done for other modules that provide permissions callbacks. Can we get a followup meta for that?

Thanks everyone!

This will also need a change record. Thanks!

Fixed the code review issues specified in #35

There was an issue with the above patch please ignore it

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Moving back to NW for the change record and follow up mentioned in #35

Updated the DI in permission callbacks for TaxonomyPermissions.php, a change record is still needed.

> This will also need a change record. Thanks!

I don't think this needs a CR - the class is internal. It's not even a service.

Rebased, updated to use autowiring and constructor property promotion, changed to not inject entity storage, added CR and deprecation message although I somewhat agree this is overkill.

Opened #3516433: META: Use dependency injections in permission callbacks to track others

Change here though seems pretty straightforward though, good deprecation.

LGTM

Setting to needs work for an item in the MR. Also, this needs a title update.

I see that xjm asked for a Meta to make these changes in other permission callback but I wonder if these can all be done at once. Not sure.

I updated the title and took care of the cr link.

Title seems good now, change still looks good.

Committed/pushed to 11.x, thanks!

I jumped the gun and deleted the CR because we don't need change records for constructor bc, then realised that we do need them because phpcs demands one for all depecation messages. I really think we need to change that requirement, but for now I made a new CR and a redirect from the old one to the new one.

Amending attribution.