Path module calls getInternalPath without checking if the url is routed

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

#2 didn't seem to break anything and changes seem innocent enough. Think we good to start the tests?

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.

The last submitted patch, 7: 3342398-7.patch, failed testing. View results →

The last submitted patch, 8: 3342398-8-FAIL.patch, failed testing. View results →

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 :)

The last submitted patch, 13: 3342398-13-FAIL.patch, failed testing. View results →

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.

Test coverage seems good and the change makes sense.

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.

Will see what the committer thinks. Also like reusing what's there if possible. But if we do need coverage for that could we still reuse that entity and make a new one for the one scenario? Seems overkill just speaking out loud.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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.

Rebased the patch for 11.x

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.

Just a couple of nits but one I'm not sure how to fix on commit.

1. +++ b/core/modules/path/tests/src/Functional/PathEntityTestExternalFormTest.php
   @@ -0,0 +1,57 @@
   +
   +  /**
   +   * Tests the node form ui.
   +   */
   

   Should this be Path form UI as in the class phpdoc? LOoks like it could be copypasta.

2. +++ b/core/modules/path/tests/src/Unit/Field/PathFieldItemListTest.php
   @@ -0,0 +1,125 @@
   +
   +    // Dependency injection is not used twice within the scope of the covered
   +    // code. Mock the field type manager and path alias repository and place
   +    // use $path_field_list when mocking $field_type_manager.
   +    $field_type_manager = $this->createMock('Drupal\Core\Field\FieldTypePluginManagerInterface');
   

   'place use' looks like an editing typo but I'm not sure what it's actually describing here.

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.

Seems points in 28 have been addressed

The last submitted patch, 29: 3342398-29.patch, failed testing. View results →

That was an unrelated fail being tracked here: 🐛 Random test fail in Drupal\Tests\Component\Utility\RandomTest::testRandomMachineNamesUniqueness Needs work

Back to RTBC.

The last submitted patch, 29: 3342398-29.patch, failed testing. View results →

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

I'm triaging RTBC issues → . I re-read the IS and the comments. I didn't find any unanswered questions or other work to do.

I read the Issue Summary and the comments. Of special note is the details on what is not being tested in #20.

I skimmed that patch and this error message caught my eye.

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

Since this will show in the UI I think it should be easier for an admin to understand and to fix. And it should not continue to use 'unrouted' which is a misspelled word in the dictionary. From the IS I gather that this is an uncommon case. Therefore, how about we at least avoid the spelling error and make a followup, tagged usability, to improve the message. Unless someone has a better idea how about, "An entity without a route cannot have a path."?

Actually, I have a moment and I will update the patch. Tagging for a follow up.

The last submitted patch, 35: 3342398-35.patch, failed testing. View results →

Completely forgot about the test.

Opened https://www.drupal.org/project/drupal/issues/3384376 📌 Improve message in PathWidget Active as a follow up

The last submitted patch, 37: 3342398-37.patch, failed testing. View results →

Thanks for the this, just what I needed.

dpi → changed the visibility of the branch 3342398-path-module-calls to hidden.

dpi → changed the visibility of the branch 11.x to hidden.

Fixed conflicts for 10.2 and 11.x and created a MR for 11.x

Small IS rewrite.

samit.310@gmail.com → made their first commit to this issue’s fork.

Hi @dpi,

I observe a issue, that post enabling Path entity_test_external module, The URL alias field value is not getting saved while saving the entity_test_external entity.

attaching the Screenshot.

Thanks
Samit K.

@samit My understanding of the test is that the entity should not be able to be saved, and so the alias does not get set, because it does not have a route (the test checks for the error message stating that).

If this still passes tests it should be back to RTBC.

The 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.