Problem/Motivation

\Drupal\Core\Entity\Entity::urlRouteParameters() calls $this->getRevisionId() when called with the revision link template even if $this does not implement \Drupal\Core\Entity\RevisionableInterface.

The similar code in Entity::toUrl() has the proper check:

    if ($rel === 'revision' && $this instanceof RevisionableInterface && $this->isDefaultRevision()) {
      $rel = 'canonical';
    }

Proposed resolution

Add a check for RevisionableInterface.

Test coverage for this is blocked on #2751395: Rewrite EntityUrlTest.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#4 2751583-3.patch1.3 KBtstoeckler
#4 2751583-3--test-only.patch713 byteststoeckler
#2 2751583-2.patch622 byteststoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Active » Needs review
FileSize
2751583-2.patch622 bytes

Here we go.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Issue summary: View changes
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Title: [PP-1] Entity::urlRouteParameters() calls getRevisionId() for non-revisionable entities » Entity::urlRouteParameters() calls getRevisionId() for non-revisionable entities
FileSize
2751583-3--test-only.patch713 bytes
2751583-3.patch1.3 KB

Yay, #2751395: Rewrite EntityUrlTest was committed. So now with comes with test coverage. The --tests-only patch is also the interdiff.

I wanted to add a comment before the line in the test to explain why we use the revision template there, but didn't manage to find proper words without writing an entire essay. Also, if people look at the actual code that is being run, it should be fairly evident, so maybe it's OK like this...

The last submitted patch, 4: 2751583-3--test-only.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Also, if people look at the actual code that is being run, it should be fairly evident, so maybe it's OK like this...

Yeah, one could even argue that its already enough that phpstorm complains about it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 7742f48 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 8ce4b6b on 8.2.x 
    Issue #2751583 by tstoeckler: Entity::urlRouteParameters() calls...

  • alexpott committed 7742f48 on 8.1.x 
    Issue #2751583 by tstoeckler: Entity::urlRouteParameters() calls...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.