Issue #3579655: Skip per-id pattern cache for new entities to avoid using NULL as an array offset (deprecated in PHP 8.5).

Frank Mablyrequested to merge
issue/pathauto-3579655:3579655-skip-cache-for-new-entities into 8.x-1.x

Symptom

Under PHP 8.5, calling PathautoGenerator::getPatternByEntity() with an unsaved (new) entity emits Deprecated: Using null as an array offset. The new-entity path is exercised in production via PathautoState::getValue() (default value lookup for the pathauto field property) and via PathautoWidget::formElement() (node add form rendering), so any add-form on PHP 8.5 spams the deprecation log.

Cause

The pattern cache is keyed by $this->patterns[$entity_type_id][$entity_id][$langcode]. For a new entity, $entity->id() is NULL. The original code wrote and read into $this->patterns[$entity_type_id][NULL][$langcode], which PHP 8.5 deprecates.

Alternative to !169

!169 fixes the symptom by coercing the NULL id to 0 and the empty langcode to '' via $entity_id = $entity->id() ?? 0 / $langcode = $entity->language()->getId() ?: ''. That works, but it silently caches every new entity under the same [$entity_type_id][0] slot. In a single request that creates several new entities of the same type, the second-and-subsequent lookups would return the first one's cached pattern regardless of applies(). The existing if ($entity->isNew()) { return $pattern; } early-return inside the loop already shows the original intent: new entities should not participate in the per-id cache at all.

This MR makes that intent explicit by short-circuiting at the top of the method:

if ($entity->isNew()) {
  foreach ($this->getPatternByEntityType($entity_type_id) as $pattern) {
    if ($pattern->applies($entity)) {
      return $this->applyLanguagePatternOverride($pattern, $langcode);
    }
  }
  return NULL;
}

For saved entities the cache logic is unchanged; the language-override block was extracted into a small applyLanguagePatternOverride() helper to avoid duplicating it between the new-entity and cached branches.

Why this approach

  • No NULL ever reaches an array offset, so the PHP 8.5 deprecation is gone at the source rather than masked by coercion.
  • No collision risk between distinct unsaved entities of the same type within a single request.
  • Matches the pre-existing $entity->isNew() intent (don't cache new entities) — the original code already had that branch inside the loop, this just lifts it to the top so it controls cache participation, not just the return site.

Behaviour preserved

Full PathautoKernelTest suite still passes locally on PHP 8.5: Tests: 22, Assertions: 421. The only test-suite deprecation reported is unrelated (token_requirements without #[LegacyRequirementsHook], tracked separately in #3549685).

The new-entity code path is already exercised by PathautoKernelTest::testTaxonomyPattern() through PathautoTestHelperTrait::assertEntityPattern() (line 229), which creates an unsaved entity and calls getPatternByEntity() on it. Drupal's error handler routes PHP's runtime E_DEPRECATED to the logger rather than failing PHPUnit (PHPUnit only fails on E_USER_DEPRECATED), which is why CI on PHP 8.5 doesn't go red — but the deprecation is still emitted on every node-add request in production.

Maintainer choice

Posted as a sibling MR to !169 so the maintainer can pick whichever framing they prefer. !169 is a smaller diff (two coercions); this MR is a slightly bigger diff that removes the deprecation at the structural level and avoids the shared-0-slot caching question. Happy to close this if !169 is preferred, or to fold these ideas back into !169.

Merge request reports