Path module calls getInternalPath without checking if the url is routed

Here's a patch. I'm adding the Needs tests tag.

It took all day, but I have tests! I also improved upon the patch in #2 a bit.

I started with a Unit test for PathFieldItemList, which was brutal. Then I made a functional test that leverages the entity_test_external entity type that was added as part of tests for #2914785: Entities with external urls as a uri relationship can not be deleted when menu_link_content is installed. The behavior of the entity_test_external entity type is extremely similar to what my snippet in the IS does:

  public function toUrl($rel = 'canonical', array $options = []) {
    if ($rel === 'canonical') {
      return Url::fromUri('http://example.com', $options);
    }
    return parent::toUrl($rel, $options);
  }

So I didn't have to make a new entity class, but I did have to add a few things to the annotation so that I could get add and edit forms.

It turns out the functional test exposes the same bug exposed by the Unit test. So I'm not sure the Unit test provides a ton of value. It's there if we want it, but I'd be fine if we were to decide to remove it from the patch.

The Functional test exposes several bugs in series, but it can obviously only fail once at a time. So here I'll take you through how each hunk of the fix causes the test to get farther before failing.

If I run the tests with no fix, I get this failure error when trying to create the entity:

UnexpectedValueException: Unrouted URIs do not have internal representations. in Drupal\Core\Url->getInternalPath() (line 804 of core/lib/Drupal/Core/Url.php).
Drupal\path\Plugin\Field\FieldType\PathFieldItemList->computeValue() (Line: 34)

The hunk that fixes this is:

+++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
@@ -25,7 +25,7 @@ protected function computeValue() {
+    if (!$entity->isNew() && $entity->toUrl()->isRouted()) {

The next error I get is this when I try to edit the entity:

UnexpectedValueException: Unrouted URIs do not have internal representations. in Drupal\Core\Url->getInternalPath() (line 804 of core/lib/Drupal/Core/Url.php).
Drupal\path\Plugin\Field\FieldWidget\PathWidget->formElement(Object, 0, Array, Array, Object) (Line: 349)

The fix for that is this:

+++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
@@ -43,7 +43,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
-      '#value' => !$entity->isNew() ? '/' . $entity->toUrl()->getInternalPath() : NULL,
+      '#value' => !$entity->isNew() && $entity->toUrl()->isRouted() ? '/' . $entity->toUrl()->getInternalPath() : NULL,

Theeeeen, the test fails when I try to set a path for the entity and update the entity:

Drupal\Core\Entity\EntityStorageException: Unrouted URIs do not have internal representations. in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 816 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).
Drupal\path\Plugin\Field\FieldType\PathItem->postSave(1)

That gets fixed by the changes to `Drupal\path\Plugin\Field\FieldType\PathItem`.

After that the test would pass fine except that I thought it would be nice to add a validation error if a path is being set on an entity that isn't routed.

+++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
@@ -87,6 +87,11 @@ public static function validateFormElement(array &$element, FormStateInterface $
+      $entity = $form_state->getFormObject()->getEntity();
+      if (!$entity->isNew() && !$entity->toUrl()->isRouted()) {
+        $form_state->setError($element['alias'], t('An unrouted entity cannot have a path.'));
+      }

Without that code, there aren't any exceptions or errors, but the value of the url alias field is cleared out without any notice to the user. I think this validation error is better than that. This validation does not work if the entity is new, however. If the url alias field is populated for new unrouted entity, the content of that field simply get cleared out during PathItem::postSave(). Maybe we should add a message if that happens?

Ignore patches in #4. All the comments in #4 and #6 are still valid. Don't ignore those. :)

Oy! The violations only increase when the fix is in place. So this new fail test backs out the change to the baseline. Apologies for the mess.

Gosh, I had the wrong namespace for the new Unit test.

I need to change:

namespace Drupal\Tests\path\Unit;

to

namespace Drupal\Tests\path\Unit\Field;

Here are new fail and pass patches that only run the new unit test.

I'm going to stop trying to be cute. Let's just make correct fail and pass patches and post them. Deep breath.

Let's hope 13 is my lucky number :)

I ran across another exception when I deleted an entity like this:

Drupal\Core\Entity\EntityStorageException: Unrouted URIs do not have internal representations. in Drupal\Core\Entity\Sql\SqlContentEntityStorage->delete() (line 761 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).
Drupal\path\Plugin\Field\FieldType\PathFieldItemList->delete() (Line: 941)

So I need to add to the test such that we delete one of the external entities. And I need to fix it, of course.

Here's a patch that address what I mention in #16, both the test coverage and the fix.

Here's a patch that should apply for D9.

Thanks for the review, @smustgrave.

Test coverage seems good

There's one piece of code that I want to call out as not covered by the automated testing. It's this

+++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
@@ -91,8 +92,9 @@ public function postSave($update) {
-    elseif ($this->pid && !$this->alias) {
-      // Otherwise, delete the old alias if the user erased it.
+    elseif ($this->pid && (!$this->alias || !$entity->toUrl()->isRouted())) {
+      // Otherwise, delete the old alias if the user erased it or the entity's
+      // url has become unrouted.

It's the "if the url has become unrouted" part of that conditional. It would be possible to update the coverage to cover that, but it would require using a different entity than entity_test_external. That entity always has an unrouted url, so it can't be used to test the case where an entity has a routed url and then gets updated such that its url is unrouted. That's actually something that happens with the bundle class I describe in the IS.

I liked the idea of re-using the class that was already in the codebase for writing these tests. But it's not quite complete coverage. But I think everything else is covered in want ends up being a pretty simple functional test.

I'm also adding a related issue which is the issue for which the entity_test_external class was added.

There's something clearly wrong with the patch in #25. It's half the size of the most recent patch I posted. Let's ignore it.

I'm hoping that the review bot kicked this back because the most recent patch I uploaded was for 9.5.x. I would not expect that one to work against 10.x or 11.x. I'm going to re-upload the patch from #17, which is really the one that @smustgrave RTBC'd. Hopefully it still works.

Yes, looks like I was correct. I'm going to reset this to RTBC since it's the same patch that @smustgrave RTBC'd in #21. It just got kicked back because I uploaded a D9.5 patch in #18 and that's the one that the review-queue bot was checking.

Hi @catch,
I'm terribly sorry about those broken comments. Regarding #28.2, not even I could figure out what I was trying to say! I wonder if I deleted a whole line or two on accident? The point of the comment was to explain why I was setting a couple things on the container. So I kind of started from scratch on that comment and now I think it makes sense.

That was an unrelated fail being tracked here: #3376563: Random test fail in Drupal\Tests\Component\Utility\RandomTest::testRandomMachineNamesUniqueness

Back to RTBC.

Same fail as described in #32. Back to RTBC.