Move helpers in node_access_test.module and delete it

Failled Functional Javascript test, is not related to changes. MR is ready for review.

This is great thanks!
One comment on the MR

Also even if they seem random tests do need to pass so it's best to rerun them.

Thanks for review.
Test failed for a reason that out of the scope of the issue. Please check https://www.drupal.org/project/drupal/issues/3493406#comment-15912080 📌 Add render caching for the navigation render array Active .
Ready for review.

That probably needs to be addressed elsewhere. I'll keep an eye on this though.

Fixed at 🐛 Navigation Top Bar hides entity local tasks even if the user has no access to the bar Active commit https://git.drupalcode.org/project/drupal/-/commit/738036cc631506a9d10f4924f414280e7990632c. Ready for review.

My comments have been addressed and this looks great!

I would say for the future for these types of issues I'm not sure if i would introduce DI like this, it did make it a little harder to compare and since you have to pass it in from the container it is not super helpful on the DX side either.

Since it's just tests and opinion I don't think it should block the issue, but it will make future reviews easier if you with in other modules from the meta.

@nicxvan, thanks for your review and feedback.
I also was not sure about adding new parameter to method signature.
I checked code base and found that both approaches used:

1) \Drupal::service('entity_display.repository')
2) $this->container->get('entity_display.repository')

Next time, I will keep changes for meta's issues as close as possible to the original, to simplify review process.
And create follow-up to introduce changes that are out of scope.

Thanks for your feedback.

Sorry folks, we need a reroll here, clash in the phpstan baseline

Conflicts resolved, ready for review.

Rebase seems fine. Restoring status.

I come to a difference conclusion than @nicxvan in #8. Even though this is a test, it does not override the concerns expressed in the first paragraph of #8. I'd like this changed but I will not set to NW and will defer to another committer.

I searched and node_access_test_add_field is used in contrib, https://git.drupalcode.org/search?group_id=2&scope=blobs&search=-path%3A...

Looking at this it seems odd to be calling test module code like this from tests at all. Guessing this was added very early on along with simpletest.

Should it move to a trait instead?

I don't think the extra argument helps here because there's no real injection happening.

FWIW, I think a trait makes much more sense here.

Thanks for feedback.

I set to NW and will move node_access_test_add_field from helper class to trait.

I found 2 more contrib modules affected, not presented in @catch's link.

• https://git.drupalcode.org/project/file_to_media/-/blob/1.x/tests/src/Fu...
• https://git.drupalcode.org/project/forum/-/blob/1.x/tests/src/Functional...
• https://git.drupalcode.org/project/graphql_compose/-/blob/2.3.x/tests/sr...
• https://git.drupalcode.org/project/tracker/-/blob/1.0.x/tests/src/Functi...

Converted to trait. Remove extra argument.

Looks good to me, just twining a failed test.

Do we want to open issues in the modules affected?

Thanks for review.
Let's do it after this will be committed.

Thanks for moving this to a trait, but also the trait shouldn't be in the node_access_test namespace, I think we can move it to node or somewhere?

Thanks, moving Trait to node sounds like a good idea.
I remove comment section from trait that we had from node_access module.
I am not sure, should we save it, because part of it already exist at NodeAccessTestHook.

Change name for trait to NodeAccessTrait.
Ready for review.

Thanks that looks much more like where I'd expect it to be!

Haven't reviewed the rest of the MR yet so leaving at needs review for now.

I created a CR for this, is that enough or does this need to be deprecated like the entity one?

Personally I think the CR is enough, and maybe a quick issue in the four contrib modules found in 18.

If others think this should be deprecated and you want a hand @nikolay shapovalov let me know, it's been passed back a few times so I can do the deprecations if you want me to.

If others agree no deprecation is necessary it's RTBC from me.

@nicxvan thanks for review and creating CR. CR looks good.
Please take care of deprecation, if we will decide to use deprecation.

I think CR is enough, but let's wait for someone else feedback as well.

I reached out in slack, @larowlan maintains 2 of the projects and @andypost maintains one, they both don't mind no deprecation. @jmolivas is also pretty prolific.

I created follow ups in each project
📌 [pp-1] node_access_test_add_field has been removed use NodeAccessTrait instead Postponed
📌 [pp-1] node_access_test_add_field has been removed use NodeAccessTrait instead Postponed
📌 [pp-1] node_access_test_add_field has been removed use NodeAccessTrait instead Postponed
📌 [pp-1] node_access_test_add_field has been removed use NodeAccessTrait instead Postponed

I'll unpostpone them when this lands.

Committed/pushed to 11.x, thanks!

catch → closed merge request !10633

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