Rename RendererInterface::renderPlain() to ::renderInIsolation()

Created on 24 June 2015, over 8 years ago
Updated 13 February 2024, 12 days ago

Problem/Motivation

::renderPlain() is a weird function name.

Proposed resolution

Quoting #2450993-4: Rendered Cache Metadata created during the main controller request gets lost โ†’ :

  • renderPlain() now actually means renderInIsolation() (i.e. start its own render stack) โ€” it originally meant "turn this render array into a plain string, and don't care about anything else". But it did that by starting a new stack, and then restoring the old one. So the "getting a plain string" part was only a symptom, what it really is about, is rendering in isolation.
  • Remaining tasks

    Do it.

    User interface changes

    None.

    API changes

    Rename RendererInterface::renderPlain() to ::renderInIsolation(), but with BC compatibility layer.

    Data model changes

    None.

    ๐Ÿ“Œ Task
    Status

    RTBC

    Version

    11.0 ๐Ÿ”ฅ

    Component
    Renderย  โ†’

    Last updated 2 days ago

    Created by

    ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Live updates comments and jobs are added and updated live.
    Sign in to follow issues

    Merge Requests

    Comments & Activities

    Not all content is available!

    It's likely this issue predates : some issue and comment data are missing.

    • First commit to issue fork.
    • Status changed to Needs review 3 months ago
    • ๐Ÿ‡ฌ๐Ÿ‡ทGreece dimitriskr

      I don't know if that is still relevant but I've switched the patch in #1 to a MR, added back the renderPlain() with some deprecation messages and switched again the code for more calls of this method. Also a draft CR is ready

    • Pipeline finished with Failed
      3 months ago
      Total: 515s
      #62218
    • Status changed to Needs work 3 months ago
    • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

      Seems to have some failures in the MR.

      Deprecation can version can be 10.3 and removed in 11.x

    • Pipeline finished with Failed
      3 months ago
      #62278
    • Pipeline finished with Failed
      3 months ago
      #62282
    • Status changed to Needs review 3 months ago
    • ๐Ÿ‡ฌ๐Ÿ‡ทGreece dimitriskr

      Thank you @smustgrave!

      I don't know why the test-only job fails, the error is that it cannot find the renderInIsolation() method. And I cannot trigger the other jobs

      Also for the spellcheck failure, shall I apply the following change? It capitalizes one letter

          */
      -  public function testRenderRecursionWithNestedrenderInIsolation() {
      +  public function testRenderRecursionWithNestedRenderInIsolation() {
           [$complex_child_markup, $parent_markup, $complex_child_template] = $this->setUpRenderRecursionComplexElements();
      
      
    • Status changed to Needs work 3 months ago
    • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

      The R should be capitalized, tests aren't running because of the spelling error.

    • Status changed to Needs review 3 months ago
    • ๐Ÿ‡ฌ๐Ÿ‡ทGreece dimitriskr
    • Pipeline finished with Success
      3 months ago
      Total: 637s
      #62293
    • ๐Ÿ‡ฌ๐Ÿ‡ทGreece dimitriskr

      All green now, thanks again @smustgrave

    • Status changed to Needs work 2 months ago
    • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

      Left a few nitpicks, but believe we should add a test that checks the deprecated message.

      All instances were replaced though, did a search for renderPlain()

    • First commit to issue fork.
    • ๐Ÿ‡ฎ๐Ÿ‡ณIndia ankithashetty Karnataka, India
    • Pipeline finished with Success
      2 months ago
      Total: 741s
      #62710
    • Status changed to Needs review 2 months ago
    • ๐Ÿ‡ฌ๐Ÿ‡ทGreece dimitriskr
    • Pipeline finished with Success
      2 months ago
      Total: 556s
      #62740
    • Status changed to RTBC 2 months ago
    • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

      Test looks good!

    • Status changed to Needs work 2 months ago
    • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

      It's still under discussion, but given the amount of changes in core and also usages in contrib, and that 11.0 and 10.3 could be released on the same day, and because the bc layer is just the old function calling the new one, this is the kind of deprecation I think we might now want to do for removal in Drupal 12 instead of 11 so that it doesn't block contrib compatibility with Drupal 11.

    • Pipeline finished with Success
      2 months ago
      Total: 604s
      #62791
    • Status changed to Needs review 2 months ago
    • ๐Ÿ‡ฌ๐Ÿ‡ทGreece dimitriskr

      Moved removal for 12.0.0

    • Status changed to RTBC 2 months ago
    • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

      Remarking but per slack conversation https://drupal.slack.com/archives/C04CHUX484T/p1702418568197489 this is dependent on what the strategy going forward for 10.3 and 11.x will be.

    • Status changed to Needs work about 2 months ago
    • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone New Zealand

      I applied the diff and searched, $ grep -r renderPlain core, and found some instances in comments that need to be changed. Setting to Needs work for those.

      I read the CR and it is short and concise which is fine in this case.

    • Pipeline finished with Success
      about 2 months ago
      #73802
    • Status changed to Needs review about 2 months ago
    • Status changed to Needs work about 2 months ago
    • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

      MR is not mergable

      Have not re-reviewed yet.

    • Pipeline finished with Failed
      about 2 months ago
      #74387
    • Pipeline finished with Success
      about 2 months ago
      Total: 621s
      #74397
    • Status changed to Needs review about 2 months ago
    • ๐Ÿ‡ฌ๐Ÿ‡ทGreece dimitriskr

      I rebased yesterday but it wasn't enough
      I also found more findings

    • ๐Ÿ‡จ๐Ÿ‡ฆCanada Charlie ChX Negyesi ๐ŸCanada

      it originally meant "turn this render array into a plain string,

      It certainly has confused this old ghost a few times when this method returned HTML instead of plain text.

      Strong support for renaming it.

    • Status changed to RTBC about 2 months ago
    • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

      Applied the MR and did a search for renderPlain() and all instances minus deprecation have been replaced.

    • Status changed to Needs work 19 days ago
    • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone New Zealand

      The title of the issue implies that this is still under discussion. Can someone change the title so it reflects the change being made?

      Thanks

    • Status changed to Needs review 12 days ago
    • ๐Ÿ‡ฌ๐Ÿ‡ทGreece dimitriskr
    • Status changed to Needs work 12 days ago
    • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

      Appears to no longer be mergable

    • ๐Ÿ‡ฌ๐Ÿ‡ทGreece dimitriskr

      dimitriskr โ†’ changed the visibility of the branch 11.x to hidden.

    • Pipeline finished with Success
      12 days ago
      Total: 571s
      #94083
    • Status changed to Needs review 12 days ago
    • ๐Ÿ‡ฌ๐Ÿ‡ทGreece dimitriskr

      Now it's ready again

    • Status changed to RTBC 12 days ago
    • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

      Rebase appears good.

    Production build https://api.contrib.social 0.61.6-2-g546bc20