Problem/Motivation

#2666798: Add a entity revision parameter converter & route enhancer introduced a generic entity revision parameter converter, so this could be leveraged by the node module.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#30 2678492-30.patch4.7 KBkevin.dutra
#28 2678492-28.patch4.59 KBSteven Spasbo
#24 2678492-24.patch4.57 KBjhedstrom
#24 interdiff-2678492-18-24.txt381 bytesjhedstrom
#18 2678492-18.patch4.53 KBamateescu
#6 2678492.patch4.54 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Related issues: +#2666798: Add a entity revision parameter converter & route enhancer
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

fwiw based on the discussion in #2678492: Convert the node revision converter to the generic entity one I'd be OK with the change in a minor release, but it feels like something for 8.2.x when that branch is open. That gives us plenty of time to revert or for affected modules to update if the theoretical API change turns out to be a real one.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
2678492.patch4.54 KB

Let's give this a try :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented 
+++ b/core/modules/node/src/Controller/NodeController.php
@@ -118,15 +118,14 @@ public function add(NodeTypeInterface $node_type) {
-   * @param int $node_revision
-   *   The node revision ID.
...
+  public function revisionShow(NodeInterface $node_revision) {

I just ask myself whether this is okay API break. Code on that page which uses $route_match->getParameter('node_revision') would now get an object instead of the ID.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

Well, if it's not ok, than this issue is won't fix.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Yeah, let's ask catch.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen as a volunteer commented

Fixing this would break B/C, but leaving this unfixed breaks stuff too. This inconsistency is currently breaking contrib. OG has a context provider that loops over the entity link templates and resolves the entity from the route. This works great for all core routes, except for the node revision routes because they are not upcasting correctly, it gets a string ID where it expects an entity :(

Ref. https://github.com/Gizra/og/blob/8.x-1.x/src/OgRouteGroupResolverBase.ph...

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen as a volunteer commented

@catch mentioned in comment #3 that he would be fine with this being fixed in a minor release. That's a very long time ago though. @catch can you confirm your comment or do we postpone until 9.0.0?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

I still think it's OK to fix this, it's not part of the public API, and as #10 shows the existing behaviour breaks some expectations anyway.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

So.. given #12, I think we're good for RTBC here?

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

@amateescu - yes, looks good to me!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2678492.patch, failed testing. View results

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs work » Reviewed & tested by the community
FileSize
2678492-18.patch4.53 KB

The patch needed a reroll for the conversion to short array syntax.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

+1 based upon #12

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change notice

Can we get a change notice here? In particular pointing out the change to route match.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

@larowlan, not sure what you mean, which route match is changed by this patch?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Sorry, see comment 7

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented

I just stumbled across this issue when I realized that the node parameter was not being converted to an object on the revision route. Can that also be addressed here, or should that be a new issue? That seemed like more of a bug to me since the route clearly defines {node} in the path.

The change to the patch to convert that to an object is as simple as:

diff --git a/core/modules/node/node.routing.yml b/core/modules/node/node.routing.yml
index 88fe4e6..f8ae66e 100644
--- a/core/modules/node/node.routing.yml
+++ b/core/modules/node/node.routing.yml
@@ -61,6 +61,8 @@ entity.node.revision:
     node: \d+
   options:
     parameters:
+      node:
+        type: entity:node
       node_revision:
         type: entity_revision:node
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
FileSize
interdiff-2678492-18-24.txt381 bytes
2678492-24.patch4.57 KB

Adding the fix mentioned in #23 for now. This can be ignored if folks think a follow-up is better (and then 18 is still RTBC with a change notice).

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented

I think as well that we should have a change notice about the route match parameter being entity now, but I don't consider it as BC break, because usually when a raw parameter is to be retrieved then one should use \Drupal\Core\Routing\RouteMatchInterface::getRawParameter() instead of \Drupal\Core\Routing\RouteMatchInterface::getParameter() which by definition is returning the processed value of a named route parameter.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Steven Spasbo’s picture

Steven Spasbo CreditAttribution: Steven Spasbo at Workday, Inc. commented
FileSize
2678492-28.patch4.59 KB

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kevin.dutra’s picture

kevin.dutra CreditAttribution: kevin.dutra at Workday, Inc. commented
FileSize
2678492-30.patch4.7 KB

Re-roll for 8.8.x

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Issue tags: -Needs change notice +Needs change record
johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented
Issue tags: -Needs change record

Added change record: https://www.drupal.org/node/3093408

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer and at PreviousNext commented
Status: Needs review » Closed (duplicate)

Closing this as a duplicate of #2730631: Upcast node and node_revision parameters of node revision routes as the patch there has test and issue has more followers as well. I'll transfer the credit from here over there.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer and at PreviousNext commented

Marking change notice as duplicate as well of https://www.drupal.org/node/2942013.