Follow-up to #2376791: Move all _content routing definitions to _controller.

This issue left in a bc layer for _content, in this issue we should

1. Remove it (to avoid having to maintain a backwards compatibility layer and its tests for 4-6 years - this is a real BC layer not a wrapper).

2. Figure out when the removal happens

3. Document the removal.

API changes:

_content no longer works in routing.yml, updating is a simple find and replace to _controller

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because we're removing an ambiguous way of defining a route, not adding new functionality.
Issue priority Critical because upcasting entities are broken for _content routes.
Major because this would remove an unneeded bc layer (and already partially broken) that would we have to maintain for 3-6 years. Not critical because leaving in the code won't break anything.
Prioritized changes The main goal of this issue is to remove code that is not needed after #2376791: Move all _content routing definitions to _controller. This is followup to a major, so it is a prioritized change for the beta phase.
Disruption
  • BC break for contrib modules
  • Some contrib modules will have to upgrade their routing YAML.
  • The change is an easy one to make. A change record exists here https://www.drupal.org/node/2378809
    		•
    CommentFileSizeAuthor
    #6 2377967-6.patch2.01 KBdawehner

    Comments

    catch’s picture

    catch
    he/him
    Primary language English
    commented

    Follow-up to #2376791: Move all _content routing definitions to _controller.

    This issue left in a bc layer for _content, in this issue we should 1. Remove it 2. Figure out when the removal happens 3. Document the removal.

    catch’s picture

    catch
    he/him
    Primary language English
    commented
    Issue tags: +D8 upgrade path

    So my personal view on this is we should go ahead and remove the bc layer sooner rather than later. An actual bc layer (as opposed to a wrapper) is real code we have to maintain for 3-6 years along with test coverage and while some contrib modules will have to upgrade their routing YAML it's a simple change to make.

    Tagging D8 upgrade path since this will likely break an existing site without at minimum a route rebuild though.

    Crell’s picture

    Crell commented

    Agreed with #2. Fairly small pain now to save lots of ongoing pain for the next several years.

    wim leers’s picture

    wim leers
    Location Ghent 🇧🇪🇪🇺
    commented

    #2++

    catch’s picture

    catch
    he/him
    Primary language English
    commented

    Remaining tasks:

    • Write a draft change notice for the _content -> _controller change for 8-8
    • Update the handbook routing.yml examples
    • Write a patch to remove the bc layer
    dawehner’s picture

    dawehner
    Primary language German
    commented
    Status: Postponed » Needs review
    StatusFileSize
    new 2377967-6.patch2.01 KB
    wim leers’s picture

    wim leers
    Location Ghent 🇧🇪🇪🇺
    commented
    Status: Needs review » Reviewed & tested by the community

    dawehner++

    Slightly refined the CR.

    alexpott’s picture

    alexpott
    he/they
    Primary language English
    Location 🇪🇺🌍
    commented
    Status: Reviewed & tested by the community » Needs work
    Issue tags: +Needs issue summary update

    There's a distinct lack of an issue summary :)

    catch’s picture

    catch
    he/him
    Primary language English
    commented
    Issue summary: View changes
    catch’s picture

    catch
    he/him
    Primary language English
    commented
    Issue summary: View changes
    catch’s picture

    catch
    he/him
    Primary language English
    commented
    Status: Needs work » Reviewed & tested by the community

    Guess who filled out the comment field instead of the issue summary field when posting #1 - fixed it.

    webchick’s picture

    webchick
    she/they
    Primary language English
    Location Vancouver 🇨🇦
    commented

    Have a poll going on Twitter about this atm: https://twitter.com/webchick/status/535450017134153728

    webchick’s picture

    webchick
    she/they
    Primary language English
    Location Vancouver 🇨🇦
    commented

    The consensus so far from the internets is break it sooner than later. Since the change record just went up a few hours ago, maybe we'll hold off until Monday just to give people some time to update?

    webchick’s picture

    webchick
    she/they
    Primary language English
    Location Vancouver 🇨🇦
    commented

    Actually I lied. :P The change record was there, but unpublished so just published https://www.drupal.org/node/2378809 now.

    webchick’s picture

    webchick
    she/they
    Primary language English
    Location Vancouver 🇨🇦
    commented

    Hm. In writing up #2379243: Adjust for the _content -> _controller change in routing.yml files I'm wondering if right before beta4 is best. Committing it 2+ weeks out from the next D8 beta leaves modules in a tricky place, since there's no way to say "I now require HEAD" in your .info.yml file, and there's no stable release to peg it to. Basically, the only workaround I can think of is doing some kind of nasty pseudo-BC layer in every module that then needs to be ripped out in beta4.

    alexpott’s picture

    alexpott
    he/they
    Primary language English
    Location 🇪🇺🌍
    commented
    Status: Reviewed & tested by the community » Needs work

    This issue is a major task. To evaluate whether we should make this change during the beta, we need to update the issue summary according to https://www.drupal.org/core/beta-changes to explain what improvements it makes and what the disruption of the change would be. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

    lokapujya’s picture

    lokapujya
    Primary language English
    Location Boston
    commented
    Issue summary: View changes

    Added the template.

    lokapujya’s picture

    lokapujya
    Primary language English
    Location Boston
    commented
    Issue summary: View changes
    lokapujya’s picture

    lokapujya
    Primary language English
    Location Boston
    commented
    Issue summary: View changes
    lokapujya’s picture

    lokapujya
    Primary language English
    Location Boston
    commented
    Issue summary: View changes
    dawehner’s picture

    dawehner
    Primary language German
    commented
    Issue summary: View changes
    Priority: Major » Critical

    Given that change (its actually broken atm.) I would consider this as critical.

    _content no longer upcasts entities, I'm sorry for that :(

    dawehner’s picture

    dawehner
    Primary language German
    commented
    Issue summary: View changes
    dawehner’s picture

    dawehner
    Primary language German
    commented
    Status: Needs work » Needs review

    Sets to needs review.

    webchick’s picture

    webchick
    she/they
    Primary language English
    Location Vancouver 🇨🇦
    commented
    Status: Needs review » Reviewed & tested by the community
    Issue tags: -Needs issue summary update

    I believe what's there is now sufficient. Tentatively moving back to RTBC.

    alexpott’s picture

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

    The general consensus is break sooner than later so... Committed 80d0419 and pushed to 8.0.x. Thanks!

    Thanks for adding the beta evaluation for to the issue summary.

    • alexpott committed 80d0419 on 8.0.x 
      Issue #2377967 by dawehner: Remove bc layer for _content _controller...
    jhodgdon’s picture

    jhodgdon
    she/her
    Primary language English
    commented

    We need a follow-up issue to fix some documentation. Filed: #2381509: Fix docs for _content being _controller in routing.yml files

    Status: Fixed » Closed (fixed)

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