Discovered in #3569421: Remove remaining unnecessary uses of Formattable markup in tests that these methods are no longer used.
Maybe these became used in #3041863: Convert a few node access tests from functional to kernel tests. In that issue both \Drupal\Tests\node\Kernel\NodeAccessTestBase::assertNodeAccess() and \Drupal\Tests\node\Kernel\NodeAccessTestBase::assertNodeCreateAccess() were created.
| Comment | File | Size | Author |
|---|
Show commandsStart 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:
changes, plain diff MR !16198
Comments
Comment #2
quietone commentedComment #4
quietone commentedNo tests failed so setting for a review
Comment #5
mondrakeSeems a no brainer for core, question would be if contrib uses the assertions and if so, whether we want to keep supporting that. But anyway 2 out of 3 methods are marked internal.
Comment #6
godotislateAccess by Taxonomy in contrib uses
assertNodeAccessquite a bit: https://git.drupalcode.org/project/access_by_taxonomy/-/blob/1.0.x/tests...We could probably open an issue in that project. Since the core method is so small, it should be a straightforward fix.
Comment #7
godotislateComment #8
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #9
dcam commentedI'm the one who originally discovered the "dead" Functional assertions and suggested they might be deleted. I didn't think to check for contrib uses at the time. I double-checked #6 because that linked test base is a Kernel test and wouldn't be affected by the Functional assertion deletion. But Access By Taxonomy does implement it in other Functional tests, as does Domain Access per https://search.tresbien.tech/search?q=assertNodeAccess.
This discovery got me thinking about the issue. Maybe these assertions should be moved to a trait instead. The functions are only slightly out of sync: it looks like the
assertEquals()expected/actual parameters were swapped in either the Functional or Kernel version at some point. If we do that, then we could haveFunctional\NodeTestBaseimplement the trait. That would be less disruptive for downstream code while still accomplishing a goal of eliminating duplicate code.Comment #10
godotislateI missed that the usages in Access by Taxonomy are in kernel tests. And looking at the search results again, all the results are in Kernel tests.
I think domain access is fine because they define their own
assertNodeAccessmethod and do not inherit from core's. https://git.drupalcode.org/project/domain/-/blob/4147c874488175b546b4737...If someone else can validate these two things, we can go back to RTBC after a rebase.
Comment #11
dcam commentedYes, you're right. I really thought that I saw a Functional test from Access By Taxonomy, but obviously got confused somewhere along the line. And Domain Access does implement its own function. I'm sorry for the noise.
So much for trying to be thorough.
Comment #12
dcam commentedI rebased it. Per #10 I'm setting it back to RTBC.
Comment #13
longwaveDead code doesn't really harm anything so let's leave this in 11.x on the off chance someone is using them somewhere.
Committed and pushed 40680fe8869 to main. Thanks!