Problem/Motivation

The REQUEST event handler method is named onRouteMatch which is very confusing.

Proposed resolution

Rename it to onRequest.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#22 2575841-22.patch1.89 KBMerryHamster
#18 use_proper_method_name-2575841-18.patch1.89 KBUtkarsh_Mishra
#12 use_proper_method_name-2575841-12.patch1.87 KBmarkdorison
#3 use_proper_method_name-2575841-3.patch2.23 KBnicrodgers
#2 use_proper_method_name-2575841-2.patch2.19 KBznerol
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol created an issue. See original summary.

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Active » Needs review
Issue tags: +Quick fix, +Quickfix
FileSize
use_proper_method_name-2575841-2.patch2.19 KB
nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
CreditAttribution: nicrodgers commented
FileSize
use_proper_method_name-2575841-3.patch2.23 KB

Patch #2 fails to apply to the latest 8.0x-dev in git, so I've re-rolled it for the current version.

dawehner’s picture

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

+1

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: use_proper_method_name-2575841-3.patch, failed testing.

nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
CreditAttribution: nicrodgers commented

patch #3 still applies to 8.0.x, and although #5 says it failed testing, I can't see any links to a failed test report? What am I missing?

nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
CreditAttribution: nicrodgers commented
Status: Needs work » Needs review

triggering another testbot review.

nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
CreditAttribution: nicrodgers commented
Status: Needs review » Reviewed & tested by the community

Tests passed, so the previous failure must have been a hiccup with the test suite. Marking RTBC again as per #4.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: use_proper_method_name-2575841-3.patch, failed testing.

skyredwang’s picture

skyredwang
Location Shanghai
CreditAttribution: skyredwang at INsReady commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: use_proper_method_name-2575841-3.patch, failed testing.

markdorison’s picture

markdorison
he/him
CreditAttribution: markdorison at Chromatic commented
Status: Needs work » Needs review
FileSize
use_proper_method_name-2575841-12.patch1.87 KB

Updated against 8.1.x.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should 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.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Needs work
Issue tags: -Quick fix, -Quickfix +Needs reroll

This needs a reroll, removing the quickfix tags, as this had been ~2.5 years, that doesn't seem to qualify as quick. I'm not sure if we can still do this, for BC reasons.

Utkarsh_Mishra’s picture

Utkarsh_Mishra CreditAttribution: Utkarsh_Mishra at OpenSense Labs for DrupalFit commented
FileSize
use_proper_method_name-2575841-18.patch1.89 KB

Updated against 8.5.x.

Utkarsh_Mishra’s picture

Utkarsh_Mishra CreditAttribution: Utkarsh_Mishra at OpenSense Labs for DrupalFit commented
Status: Needs work » Needs review
savkaviktor16@gmail.com’s picture

savkaviktor16@gmail.com CreditAttribution: savkaviktor16@gmail.com at Skilld commented
Issue tags: -Needs reroll
MerryHamster’s picture

MerryHamster CreditAttribution: MerryHamster at Skilld commented
Version: 8.5.x-dev » 8.6.x-dev
MerryHamster’s picture

MerryHamster CreditAttribution: MerryHamster at Skilld commented
FileSize
2575841-22.patch1.89 KB

Here is an only reroll for 8.6.x

dawehner’s picture

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

Thank you

alexpott’s picture

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

https://www.drupal.org/core/d8-bc-policy#paramconverters are not part of the API so this is fine to change. But let's only change it in 8.6.x because nothing is actually broken.

Committed 766c804 and pushed to 8.6.x. Thanks!

  • alexpott committed 766c804 on 8.6.x 
    Issue #2575841 by nicrodgers, znerol, Utkarsh_Mishra, MerryHamster,...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Component: cache system » dynamic_page_cache.module

As the maintainer of this component, I wish I'd been given the opportunity to review this patch. It wasn't filed against the right component, hence I did not see it.

Fortunately, it's a trivial change :)

Note that this was named onRouteMatch() on purpose: it runs for the KernelEvents::REQUEST event, but runs pretty late: after routing. Hence its name.

That is in fact the very central principle about Dynamic Page Cache: that it runs immediately after routing. As its docs say:

* The reason Dynamic Page Cache is implemented as two event subscribers (a late
 * REQUEST subscriber immediately after routing for cache hits, and an early
 * RESPONSE subscriber for cache misses) is because many cache contexts can only
…

Nonetheless, +1 for this change. Consistency FTW!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Category: Bug report » Task
Priority: Normal » Minor

Status: Fixed » Closed (fixed)

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