Make sure path has a value before calling purge in pathauto_entity_delete()

what entity type is this?

The path field is expected to be computed and auto-loaded, it should always have a field item, even when there is no alias.

which drupal core version? 8.4?

I'll have a closer look at this asap. We should be doing some updates now that path is a able to load itself anyway I think, might also help wit hthis.

There were various reports since 8.4 about strange effects. Only found time now to start looking into it. Can you check if #2945734: Fix automated test failures in 8.x-1.x is fixing this for you?

If not, a failing test would be helpful.

Please provide a combined patch as well to show that it passes then.

1. +++ b/tests/src/Functional/NodeTest.php
   @@ -0,0 +1,123 @@
   +/**
   + * @group pathauto
   + */
   +class NodeTest extends BrowserTestBase {
   +
   +  public static $modules = [
   +    'language',
   

   Needs description for the class and $modules property.

2. +++ b/tests/src/Functional/NodeTest.php
   @@ -0,0 +1,123 @@
   +  protected function enableTranslation(string $entityType, string $bundle, bool $rebuild = FALSE) {
   +    if (!$this->languageInstalled) {
   +      ConfigurableLanguage::createFromLangcode('fi')
   +        ->save();
   +      $this->languageInstalled = TRUE;
   +    }
   

   why the conditional, this isn't called multiple times?

   $entityType should be $entity_type.

3. +++ b/tests/src/Functional/NodeTest.php
   @@ -0,0 +1,123 @@
   +
   +    if ($rebuild) {
   +      drupal_static_reset();
   +      \Drupal::entityTypeManager()->clearCachedDefinitions();
   

   this also seems unused?

4. +++ b/tests/src/Functional/NodeTest.php
   @@ -0,0 +1,123 @@
   +
   +  public function testWorkingNodeDelete() {
   +    $node = $this->createNodeEntity();
   +    $node->addTranslation('fi', [
   +      'title' => 'Test fi',
   +      'body' => [
   +        'value' => 'dsads',
   +        'format' => 'plain_text',
   

   there is nothing in this test that requires it to be a browser test, it should be a kernel test instead.

   One test method is also enough, and once it is fixed, it doesn't make sense to talk about working and non-working as both are working. Instead, name it after what it is testing/doing (testDeleteTranslations() or something like that)

5. +++ b/tests/src/Functional/NodeTest.php
   @@ -0,0 +1,123 @@
   +  protected function createNodeEntity() {
   +    $node = $this->entityTypeManager->getStorage('node')
   +      ->create([
   +        'title' => 'Test',
   +        'type' => 'article',
   +        'langcode' => 'en',
   +        'body' => [
   +          'value' => 'dsads',
   +          'format' => 'plain_text',
   +        ],
   +      ]);
   +    return $node;
   

   you don't really need a helper function for this, just do it inline and remove the body part, because that field doesn't exist in this test.

Good, but more test coverage never hurts, maybe that revert was temporary and then we want to make sure that it doesn't break again, so keeping open as a task to add that, which possibly is the test-only patch that you provided above..

Re-roll of #16 but moved the logic into the module file as we no longer implement delete in the field item list? Also brought back the functional test from #14.

I'm seeing this behavior in pathauto 1.3 when combined with weitzman/drupal-test-traits. The automatic cleanup happens during phpunit's tearDown phase. It only happens occasionally (still don't know exactly what causes it, but it's certain user entities).

I'm using Drupal in a headless manner (and all content is created via Entity Interfaces), I have to use this patch #21 to avoid issues.

Though, it hasn't cleanly applied to 1.x-dev for a month+ now, so I've held back at previous pathauto version.

Just chiming in to possibly help discover how/when/why this happens. (Non-UI created content?)

Patch no longer applies to latest dev.

Just a reroll from #21

Nice! I'll try to test this tomorrow.

Hi, will the patch be applied to the next version ?

1. +++ b/tests/src/Functional/NodeTest.php
   @@ -0,0 +1,144 @@
   +
   +/**
   + * @group pathauto
   + */
   +class NodeTest extends BrowserTestBase {
   ...
   +  protected $languageInstalled = FALSE;
   

   This isn't a very descriptive class name, maybe EntityDeleteTest or so?

2. +++ b/tests/src/Functional/NodeTest.php
   @@ -0,0 +1,144 @@
   +  protected function enableTranslation(string $entityType, string $bundle, bool $rebuild = FALSE) {
   +    if (!$this->languageInstalled) {
   +      ConfigurableLanguage::createFromLangcode('fi')
   +        ->save();
   +      $this->languageInstalled = TRUE;
   +    }
   

   was this method copied from somewhere?

   we only call it once, maybe just inline it into setUp()? then we don't need the dynamic language installed flag and also not the.

   And are we sure that rebuild is actually needed?

   Also looks like this could be a much faster kernel test, we don't do any web requests here?

3. +++ b/tests/src/Functional/NodeTest.php
   @@ -0,0 +1,144 @@
   +  public function testWorkingNodeDelete() {
   ...
   +  public function testBrokenNodeDelete() {
   

   working and broken aren't really useful method descriptions, because it wouldn't actually be broken anymore?

   Just one method that tests the broken part would I think also be enough.-

Hi, after applying a patch still have an issue, so I've improved it a bit.

Addressing the pointers from #28 and including the improvement from #29

🤔

"Test-only changes" doesn't fail. So not sure all this is really needed.

Code review of MR #129

The commit adds a null check in PathautoEntityHooks::entityDelete() before calling ->get('pathauto')->purge(). The original code called $entity->get('path')->first()->get('pathauto')->purge() without verifying that first() returns a valid item, which could cause a fatal error: "Call to a member function get() on null".

The fix stores $entity->get('path')->first() in a variable and checks $item instanceof PathautoItem && $item->__isset('pathauto') before proceeding. This is a correct defensive measure.

Test investigation

The test included in the MR (EntityDeleteTest) had several issues:

• It was missing the path module in $modules, and did not install the node and path_alias entity schemas or pathauto config. As a result, the path field was not available on the node entity and the delete hook's hasField('path') check returned FALSE — the test never actually exercised the fix.
• It was missing the #[RunTestsInSeparateProcesses] attribute required since Drupal 11.3.
• The reproduction scenario (create node, add translation, save, delete) does not trigger the bug in current Drupal 11 because ComputedItemListTrait::get() always calls ensureComputedValue(), so first() returns a valid item.

After fixing the test setup, the only way to make first() return NULL is to artificially clear the path field with $node->get('path')->setValue([]) before deletion. This marks the computed value as already resolved while emptying the item list. With this approach, the test correctly crashes without the fix ("Call to a member function get() on null") and passes with it — but it does not correspond to a real-world use case.

Conclusion

The fix itself is correct and harmless as a defensive null check. However, the original bug may no longer be reproducible in Drupal 11 due to changes in ComputedItemListTrait. The test can only verify the fix by artificially creating the null condition.

Feel free to reopen this issue if you can provide a reproducible scenario on a fresh Drupal instance.