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.

    CommentFileSizeAuthor
    #1 consider_renaming-2511308-1.patch37.73 KBAnonymous (not verified)

    Issue fork drupal-2511308

    Command icon Show commands

    Start within a Git clone of the project using the version control instructions.

    Or, if you do not have SSH keys set up on git.drupalcode.org:

    About issue forks

    Comments

    Anonymous’s picture

    Anonymous (not verified) commented
    Status: Active » Needs review
    StatusFileSize
    new consider_renaming-2511308-1.patch37.73 KB

    I've created a patch. Please review.

    stevector’s picture

    stevector
    he/him
    commented
    Version: 8.0.x-dev » 8.1.x-dev
    Status: Needs review » Needs work

    I think it is too late to take this patch as is since it changes a method on an interface.

    cilefen’s picture

    cilefen commented 
    +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
@@ -57,7 +57,7 @@ public function renderRoot(&$elements);
-  public function renderPlain(&$elements);
+  public function renderInIsolation(&$elements);

    See the issue summary:

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

    I added the emphasis. Make the BC layer by marking renderPlain() deprecated and have it call renderInIsolation().

    cilefen’s picture

    cilefen commented
    Issue tags: +Needs change record

    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.

    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.

    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.

    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.

    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.

    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.

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

    Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

    Version: 9.2.x-dev » 9.3.x-dev

    Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    Version: 9.3.x-dev » 9.4.x-dev

    Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    Version: 9.4.x-dev » 9.5.x-dev

    Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    Version: 9.5.x-dev » 10.1.x-dev

    Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    Version: 10.1.x-dev » 11.x-dev

    Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    dimitriskr made their first commit to this issue’s fork.

    dimitriskr opened merge request !5766

    dimitriskr’s picture

    dimitriskr commented
    Status: Needs work » Needs review
    Issue tags: -Needs change record

    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 searched again the code for more calls of this method. Also a draft CR is ready

    smustgrave’s picture

    smustgrave commented
    Status: Needs review » Needs work

    Seems to have some failures in the MR.

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

    dimitriskr’s picture

    dimitriskr commented
    Status: Needs work » Needs review

    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();
    smustgrave’s picture

    smustgrave commented
    Status: Needs review » Needs work

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

    dimitriskr’s picture

    dimitriskr commented
    Status: Needs work » Needs review
    dimitriskr’s picture

    dimitriskr commented

    All green now, thanks again @smustgrave

    smustgrave’s picture

    smustgrave commented
    Status: Needs review » Needs work

    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()

    ankithashetty made their first commit to this issue’s fork.

    ankithashetty’s picture

    ankithashetty
    Location Karnataka, India
    commented
    Issue tags: +Needs tests

    Tagging 'needs tests' as per #28 request. Nitpicks handled in MR.

    dimitriskr’s picture

    dimitriskr commented
    Status: Needs work » Needs review
    Issue tags: -Needs tests
    smustgrave’s picture

    smustgrave commented
    Status: Needs review » Reviewed & tested by the community

    Test looks good!

    catch’s picture

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

    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.

    dimitriskr’s picture

    dimitriskr commented
    Status: Needs work » Needs review

    Moved removal for 12.0.0

    smustgrave’s picture

    smustgrave commented
    Status: Needs review » Reviewed & tested by the community

    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.

    quietone’s picture

    quietone commented
    Status: Reviewed & tested by the community » Needs work

    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.

    dimitriskr’s picture

    dimitriskr commented
    Status: Needs work » Needs review
    dimitriskr’s picture

    dimitriskr commented
    smustgrave’s picture

    smustgrave commented
    Status: Needs review » Needs work

    MR is not mergable

    Have not re-reviewed yet.

    dimitriskr’s picture

    dimitriskr commented
    Status: Needs work » Needs review

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

    ghost of drupal past’s picture

    ghost of drupal past
    he/him
    commented

    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.

    smustgrave’s picture

    smustgrave commented
    Status: Needs review » Reviewed & tested by the community
    Issue tags: +Needs Review Queue Initiative

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

    quietone’s picture

    quietone commented
    Status: Reviewed & tested by the community » Needs work
    Issue tags: +Needs title update

    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

    dimitriskr’s picture

    dimitriskr commented
    Title: Consider renaming RendererInterface::renderPlain() to ::renderInIsolation() » Rename RendererInterface::renderPlain() to ::renderInIsolation()
    Status: Needs work » Needs review
    Issue tags: -Needs title update
    smustgrave’s picture

    smustgrave commented
    Status: Needs review » Needs work

    Appears to no longer be mergable

    dimitriskr changed the visibility of the branch 11.x to hidden.

    dimitriskr’s picture

    dimitriskr commented
    Status: Needs work » Needs review

    Now it's ready again

    smustgrave’s picture

    smustgrave commented
    Status: Needs review » Reviewed & tested by the community

    Rebase appears good.

    • catch committed cb0f209b on 10.3.x 
      Issue #2511308 by dimitriskr, ankithashetty, smustgrave, quietone, Wim...

    • catch committed 75d943a0 on 11.x 
      Issue #2511308 by dimitriskr, ankithashetty, smustgrave, quietone, Wim...
    catch’s picture

    catch
    he/him
    Primary language English
    commented
    Version: 11.x-dev » 10.3.x-dev
    Status: Reviewed & tested by the community » Fixed

    Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

    dimitriskr’s picture

    dimitriskr commented

    Can someone close the MR?

    longwave closed merge request !5766

    Status: Fixed » Closed (fixed)

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