Fix nits in \Drupal\rest\Entity\ConfigDependencies

#2: Sorry, I started addressing this already. While minor and simple stuff, you kind of need to know how all these things work to address it the appropriate way.

1. Wow, I can't believe you even noticed such a detail. @dawehner was the one who wrote that code, but AFAICT it's indeed there only to make the unit test pass. Done.
2. The intent was to use the interface were possible. Addressed by picking the first available method, and requesting formats + auth providers for that.
3. See 2.

Well, IMHO we should mimic the behaviour and ensure uniqueness actually. As someone, who builds sites, having deterministic sorting on CMI is a huge deal. It helps you a lot with actual adapting configuration quickly.

But we already have that. \Drupal\rest\Entity\ConfigDependencies::calculateDependencies() was just duplicating it. So sorting was happening twice.

Because:

1. \Drupal\rest\Entity\RestResourceConfig::calculateDependencies() does:
   

     public function calculateDependencies() {
       parent::calculateDependencies();
   
       foreach ($this->getRestResourceDependencies()->calculateDependencies($this) as $type => $dependencies) {
         foreach ($dependencies as $dependency) {
           $this->addDependency($type, $dependency);
         }
       }
       return $this;
     }
   

   (Note the addDependency() call!!!!!)

2. The called \Drupal\Core\Config\Entity\ConfigEntityBase::addDependency() does:
   

     protected function addDependency($type, $name) {
       // A config entity is always dependent on its provider. There is no need to
       // explicitly declare the dependency. An explicit dependency on Core, which
       // provides some plugins, is also not needed.
       if ($type == 'module' && ($name == $this->getEntityType()->getProvider() || $name == 'core')) {
         return $this;
       }
   
       return $this->addDependencyTrait($type, $name);
     }
   
   

   And note: that addDependencyTrait() is an aliased trait method:

     use PluginDependencyTrait {
       addDependency as addDependencyTrait;
     }
   

3. The called \Drupal\Core\Entity\DependencyTrait::addDependency() finally does:
   

     protected function addDependency($type, $name) {
       if (empty($this->dependencies[$type])) {
         $this->dependencies[$type] = array($name);
         if (count($this->dependencies) > 1) {
           // Ensure a consistent order of type keys.
           ksort($this->dependencies);
         }
       }
       elseif (!in_array($name, $this->dependencies[$type])) {
         $this->dependencies[$type][] = $name;
         // Ensure a consistent order of dependency names.
         sort($this->dependencies[$type], SORT_FLAG_CASE);
       }
       return $this;
     }
   

In other words: @effulgentsia merely pointed out that we're doing duplicate (yet incomplete, because not guaranteeing uniqueness) work. And I agree with him. That's why I added the documentation to show why \Drupal\rest\Entity\ConfigDependencies::calculateDependencies() does not need to care about either uniqueness or sorting, because that it handled later. It only provides the starting point data.

I don't think it's like that at all. What we're doing, is a set comparison. So, just made that more clear.

(Copied that over from \Drupal\big_pipe\Tests\BigPipeTest::assertSetsEqual().)

Why would they be ordered alphabetically?

Yes, and that's what I explained in #5 in detail, and what @effulgentsia wrote originally: what ConfigDependencies::calculateDependencies() returns is passed into \Drupal\Core\Entity\DependencyTrait::addDependency(), which handles sorting for us.

So this is testing the return value of ConfigDependencies::calculateDependencies(), which does not need to be sorted. Because sorting happens at a later stage.

+++ b/core/modules/rest/src/Entity/ConfigDependencies.php
@@ -209,7 +210,7 @@
-  public function onDependencyRemovalForResourceGranularity(RestResourceConfigInterface $rest_config, array $dependencies) {
+  protected function onDependencyRemovalForResourceGranularity(RestResourceConfigInterface $rest_config, array $dependencies) {

Yes, +1!

I totally agree with you that this should be an @internal class. IMHO we should do that more often :)

Yes! Did that in this reroll.

Given that its deterministic, we should, at least IMHO, reflect that in the test case but using exactly the deterministic order of dependencies we expect it to be.

It's deterministic, but you still get a different dependency order depending on whether "method" vs "resource" granularity is used. But, if we play with the test cases so that the first method already uses basic_auth, then we can make sure they end up returning the exact same ordered list of dependencies!

Yep :)

#22:

1. Done.
2. No, we are removing the need for the sort() (see third hunk in patch). This is what @dawehner asked in #7 and later comments.

Only change in this reroll: in_array(X, array) -> in_array(X, array, TRUE — i.e. strict checking. Since that's absolutely trivial, re-RTBC'ing.

Per #23, moving back to 8.2.x.

Apparently the automated re-testing hit a DrupalCI abortion, and hence this was marked NW. The patch did not actually fail. Re-testing.