Ran the module code through code sniffer and found following errors:

FILE: /var/www/drupalvm/drupal/web/modules/contrib/nodeaccess/nodeaccess.install
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 126 | WARNING | Variable $i is undefined.
--------------------------------------------------------------------------------


FILE: /var/www/drupalvm/drupal/web/modules/contrib/nodeaccess/nodeaccess.module
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 316 | WARNING | Unused variable $results.
-------------------------------------------------------------------------------


FILE: /var/www/drupalvm/drupal/web/modules/contrib/nodeaccess/src/Form/ConfigForm.php
-------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-------------------------------------------------------------------------------------------------------------
  48 | WARNING | NodeType::loadMultiple calls should be avoided in classes, use dependency injection instead
 206 | WARNING | NodeType::loadMultiple calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------------------------------------


FILE: /var/www/drupalvm/drupal/web/modules/contrib/nodeaccess/src/Form/GrantsForm.php
-------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
-------------------------------------------------------------------------------------------------
 221 | WARNING | Unused variable $preserve.
 367 | WARNING | Node::load calls should be avoided in classes, use dependency injection instead
 400 | WARNING | Unused variable $id.
-------------------------------------------------------------------------------------------------

Time: 277ms; Memory: 8MB

FILE: /var/www/drupalvm/drupal/web/modules/contrib/nodeaccess/nodeaccess.install
----------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------------
 1 | ERROR | [x] End of line character is invalid; expected "\n" but found "\r\n"
----------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------


FILE: /var/www/drupalvm/drupal/web/modules/contrib/nodeaccess/nodeaccess.module
------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
------------------------------------------------------------------------------------
   1 | ERROR | [x] End of line character is invalid; expected "\n" but found "\r\n"
 162 | ERROR | [x] Expected 1 blank line after function; 2 found
 318 | ERROR | [x] Expected 1 space after WHILE keyword; 0 found
------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------


FILE: /var/www/drupalvm/drupal/web/modules/contrib/nodeaccess/src/AccessChecks/NodeGrantAccessCheck.php
-------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------
 1 | ERROR | [x] End of line character is invalid; expected "\n" but found "\r\n"
-------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------


FILE: /var/www/drupalvm/drupal/web/modules/contrib/nodeaccess/src/Form/ConfigForm.php
-------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------
 1 | ERROR | [x] End of line character is invalid; expected "\n" but found "\r\n"
-------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------


FILE: /var/www/drupalvm/drupal/web/modules/contrib/nodeaccess/src/Form/GrantsForm.php
-------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
-------------------------------------------------------------------------------------
   1 | ERROR | [x] End of line character is invalid; expected "\n" but found "\r\n"
 166 | ERROR | [x] Inline comments must start with a capital letter
 174 | ERROR | [x] Inline comments must start with a capital letter
-------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------

Time: 498ms; Memory: 10MB
CommentFileSizeAuthor
#19 interdiff_10-19.patch1.93 KBlucasbaralm
#19 3145629-19.patch14.86 KBlucasbaralm
#15 interdiff_10-15.txt1.97 KBlucasbaralm
#15 3145629-15.patch14.79 KBlucasbaralm
#12 screen-11-23-50.png42.57 KBmichelecris
#10 interdiff_5-10.txt9.39 KBaakansha tyagi
#10 3145629-10.patch14.79 KBaakansha tyagi
#5 3145629-4.sniffer.txt535 bytesbruno.bicudo
#5 3145629-4.patch15.12 KBbruno.bicudo
#2 3145629-2.patch2.08 KBsuresh prabhu parkala

Issue fork nodeaccess-3145629

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

drupal.ninja03 created an issue. See original summary.

suresh prabhu parkala’s picture

suresh prabhu parkala commented
Status: Active » Needs review
StatusFileSize
new 3145629-2.patch2.08 KB

Please review the patch.

shashwat purav’s picture

shashwat purav commented
Status: Needs review » Needs work

The patch in #2 failed to apply successfully to 8.x-1.x branch.

bruno.bicudo’s picture

bruno.bicudo
Primary language Portuguese, Brazil
Location Bragança Paulista - São Paulo
commented
Assigned: Unassigned » bruno.bicudo

I'll work on it.

bruno.bicudo’s picture

bruno.bicudo
Primary language Portuguese, Brazil
Location Bragança Paulista - São Paulo
commented
Assigned: bruno.bicudo » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3145629-4.patch15.12 KB
new 3145629-4.sniffer.txt535 bytes

I worked on all the warnings and injected services.

Unused variables remained untouched as it's more suitable for someone with better understanding to touch them (see sniffer.txt).

Needs review :)

WagnerMelo’s picture

WagnerMelo commented
Assigned: Unassigned » WagnerMelo

I'll review it.

WagnerMelo’s picture

WagnerMelo commented
Assigned: WagnerMelo » Unassigned
Status: Needs review » Reviewed & tested by the community

I reviewed all changes that @bruno.bicudo made on #5, and evertything looks like correct for me!
I'll change this issues to RTBC.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/src/Form/ConfigForm.php
@@ -4,14 +4,41 @@ namespace Drupal\nodeaccess\Form;
+  protected $entityTypeManagerService;
...
+    $this->entityTypeManagerService = $entitytype_manager;

+++ b/src/Form/GrantsForm.php
@@ -3,15 +3,91 @@
+   */
...
+  protected $entityTypeManagerService;
...
+  protected $messengerService;
...
+  public function __construct(Connection $database_service, ConfigFactoryInterface $configfactory_service, EntityTypeManagerInterface $entitytype_manager, NodeGrantDatabaseStorage $nodegrant_storage, Messenger $messenger_service) {
+    $this->databaseService = $database_service;
+    $this->configFactoryService = $configfactory_service;
+    $this->entityTypeManagerService = $entitytype_manager;
+    $this->nodeGrantStorage = $nodegrant_storage;
+    $this->messengerService = $messenger_service;

All good, but I disagree with the variable naming. no need to use service in naming, eg, databaseService -> database. No similar naming like this in Drupal core.

Thanks!

aakansha tyagi’s picture

aakansha tyagi commented
Assigned: Unassigned » aakansha tyagi

i am working on this issue.

aakansha tyagi’s picture

aakansha tyagi commented
Assigned: aakansha tyagi » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3145629-10.patch14.79 KB
new interdiff_5-10.txt9.39 KB

I have made the changes please review it.

michelecris’s picture

michelecris commented
Assigned: Unassigned » michelecris

Hi!
I will review it!

michelecris’s picture

michelecris commented
Assigned: michelecris » Unassigned
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new screen-11-23-50.png42.57 KB

Hi!

I applied the #10 patch and searched for any variable with _service in name and all was changed. I also ran phpcs and any errors were found, just a warning in GrantsForm.php about a unused variable. So I think that is all good and l'll change the status for RTBC.

Thanks!

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/src/Form/GrantsForm.php
@@ -3,15 +3,91 @@
+  protected $nodeGrantStorage;
...
+  protected $messenger;
...
+   * @param Drupal\node\NodeGrantDatabaseStorage $nodegrant_storage
+   *   The node grant storage.
+   * @param Drupal\Core\Messenger\Messenger $messenger
+   *   The messenger.
...
+  public function __construct(Connection $database, ConfigFactoryInterface $configfactory, EntityTypeManagerInterface $entitytype_manager, NodeGrantDatabaseStorage $nodegrant_storage, Messenger $messenger) {

The last two arguments should be the respective interfaces.

lucasbaralm’s picture

lucasbaralm
He/Him
commented
Assigned: Unassigned » lucasbaralm

I will work on this!

lucasbaralm’s picture

lucasbaralm
He/Him
commented
Assigned: lucasbaralm » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3145629-15.patch14.79 KB
new interdiff_10-15.txt1.97 KB

Adding the fixes mentioned in comment #13 and adapting the order of related code to match the constructor.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

Thanks for working on this! I still don't see the changes I requested.

  1. +++ b/src/Form/GrantsForm.php
@@ -3,15 +3,91 @@
+   * @var Drupal\node\NodeGrantDatabaseStorage

    This should be NodeGrantDatabaseStorageInterface. 

  2. +++ b/src/Form/GrantsForm.php
@@ -3,15 +3,91 @@
+   * @var Drupal\Core\Messenger\Messenger

    This should be MessengerInterface. 

  3. +++ b/src/Form/GrantsForm.php
@@ -3,15 +3,91 @@
+   * @param Drupal\Core\Messenger\Messenger $messenger

    This should be MessengerInterface 

  4. +++ b/src/Form/GrantsForm.php
@@ -3,15 +3,91 @@
+   * @param Drupal\node\NodeGrantDatabaseStorage $nodegrant_storage

    This should be NGDSInterface. 

  5. +++ b/src/Form/GrantsForm.php
@@ -3,15 +3,91 @@
+  public function __construct(Connection $database, NodeGrantDatabaseStorage $nodegrant_storage, Messenger $messenger, ConfigFactoryInterface $configfactory, EntityTypeManagerInterface $entitytype_manager) {

    These should be NGDSInterface and MessengerInterface.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Needs work
lucasbaralm’s picture

lucasbaralm
He/Him
commented
Assigned: Unassigned » lucasbaralm

Sorry, I misunderstood the comment, I will work on the corrections.

lucasbaralm’s picture

lucasbaralm
He/Him
commented
Assigned: lucasbaralm » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3145629-19.patch14.86 KB
new interdiff_10-19.patch1.93 KB

Please ignore patch #15. Patch #19 corrects what was mentioned in comment #13.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

This looks good to me, thanks!

alison’s picture

alison
she/her
commented

Thank you to everyone who collaborated and worked on this issue!

alison’s picture

alison
she/her
commented

  • b7c95d9 committed on 8.x-1.x 
    Issue #3145629 by lucasbaralm, bruno.bicudo, Aakansha Tyagi, Suresh...
alison’s picture

alison
she/her
commented
Status: Reviewed & tested by the community » Fixed
lussoluca’s picture

lussoluca
he/him
Primary language Italian
Location Italy
commented
Status: Fixed » Needs work

This patch introduced a bug in the ConfigForm class:

```
- $node_types = NodeType::loadMultiple();
+ $node_types = $this->entityTypeManager->getStorage('node')->loadMultiple();
```

should be `$node_types = $this->entityTypeManager->getStorage('node_type')->loadMultiple();`

And

```
- '#default_value' => isset($user_perms['author']['grant_view']) ? $user_perms['author']['grant_view'] : 0,
+ '#default_value' => isset($user_perms['author']['grant_view']) ?? 0,
```
should be `$user_perms['author']['grant_view'] ?? 0` (and all the others like this)

Should I open a new issue for that?

pratik_specbee made their first commit to this issue’s fork.

greatmatter’s picture

greatmatter commented

I have a patch here:
https://www.drupal.org/project/nodeaccess/issues/3309679

It might be wrong given #25. Let me know and I can update the patch. Note that the ?? operator is still being used incorrectly in *this* comment stream, but not my patch.

lussoluca’s picture

lussoluca
he/him
Primary language Italian
Location Italy
commented
Status: Needs work » Fixed

Great!, I think we can re-close this and move to #3309679: Nodeaccess configuration suddenly failed

Status: Fixed » Closed (fixed)

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