Subclasses of ResourceTestBase for non-entity resources are required to add pointless code

Just realized that EntityResource already has a generic implementation of that method, so I think the abstract method can simply be removed.

Re-titling and updating the issue summary accordingly.

Hmmm... thinking about this more: Since generically, resources get their own permissions and only for entities we don't need that, would it make sense to move EntityResourceTestBase::getExpectedBCUnauthorizedAccessMessage() into ResourceTestBase as getExpectedUnauthorizedAccessMessage() (i.e. without the BC)?

I currently have this in my non-entity resource test:

  /**
   * {@inheritdoc}
   */
  protected function getExpectedUnauthorizedAccessMessage($method) {
    return "The 'restful " . strtolower($method) . " some_resource_plugin_id' permission is required.";
  }

  /**
   * {@inheritdoc}
   *
   * @todo Remove this when https://www.drupal.org/node/2869387 lands.
   */
  protected function getExpectedBcUnauthorizedAccessMessage($method) {
  }

We would only need the resource plugin ID to do this generically and EntityResourceTestBase could in turn have a default implementation of that since it knows the entity type ID.

Assigning to Wim Leers to shine some light on this, this is all very confusing to me.

Actually expanding scope a bit here, as I found another problem with ResourceTestBase: $this->serializer is declared, but never set.

Also unassigning @Wim Leers again until I have something more "presentable" here. So far I'm still thinking out loud.

Also found: #2869415: EntityResourceTestBase::getUrl() overrides BrowserTestBase::getUrl(), yet offers different functionality

Ah, awesome thank you! :-) I will keep you posted if I find anything else ;-)

Let's try this one. ResourceTestBase::$resourceConfigId conveniently has the comment:

The REST Resource plugin ID can be calculated from this.

But, I had to search a bit to verify that my assumption of how that actually works is true, so I added a @see.

+++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
@@ -62,6 +62,8 @@
+   * @see \Drupal\rest\Entity\RestResourceConfig::__construct()()

Ahh damn, double paren's...

Quick fix for #10.

Not actually sure what to with this issue in terms of the title. I agree that "does not work" is not very specific, but I have a hard time finding clearer words. Because it does actually not work, I also have a hard time writing a change notice because the "before" part is not actually clear, unless we actually want to write a change notice about people being able to remove weird workarounds they had to employ before. That seems non-standard to me, though, in terms of how we generally treat change notices. I could see a legitimate change notice for the third point below, but not sure.

In any case, not really sure what to do here, but here is some more explanation of what the parts of the patch actually mean:

1. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -104,6 +106,8 @@
   +    $this->serializer = $this->container->get('serializer');
   

   So this part is a legitimate bug and it constitutes the "does not work" part for me. Of course, the workaround is trivial (just add the serializer in your own setUp()) but still.

2. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -182,6 +186,21 @@ protected function refreshTestStateAfterRestConfigChange() {
   +  protected function getExpectedUnauthorizedAccessMessage($method) {
   +    $resource_plugin_id = str_replace('.', ':', static::$resourceConfigId);
   +    $permission = 'restful ' . strtolower($method) . ' ' . $resource_plugin_id;
   +    return "The '$permission' permission is required.";
   +  }
   
   @@ -240,29 +259,6 @@ protected function refreshTestStateAfterRestConfigChange() {
   -  abstract protected function getExpectedUnauthorizedAccessMessage($method);
   

   This is a mere nuisance as without this ResourceTestBase is less helpful than it could be and you end up implementing this again and again for every test base class in the same way. It's not actually a bug in the strict sense, though.

3. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -240,29 +259,6 @@ protected function refreshTestStateAfterRestConfigChange() {
   -  /**
   -   * Return the default expected error message if the
   -   * bc_entity_resource_permissions is true.
   -   *
   -   * @param string $method
   -   *   The HTTP method (GET, POST, PATCH, DELETE).
   -   *
   -   * @return string
   -   *   The error string.
   -   */
   -  abstract protected function getExpectedBcUnauthorizedAccessMessage($method);
   

   This is also a bug, as the whole concept of BC permissions does not make sense for non-entity resources. So you have to implement something that is not actually used. Again, the you can just implement it with return 'foo'; and be done with it, but ...

The new title is bit too forgiving for my taste, but be that as it may ;-). I don't mind this landing in 8.4.x only. If I remember correctly contrib tests on Drupal.org are run against 8.4.x anyway, so this would only be for people with their own CI on Github, etc. So marking RTBC per #24