Fatal error when passing NULL to Renderer::render()

Proposed a deprecation.

+1 for deprecating passing NULL instead

Deprecation seems like a good idea to me too.

For the deprecation

The deprecation is there. The only thing is whether we want to document on the interface that this will get a type in D12.

The only thing is whether we want to document on the interface that this will get a type in D12.

What's the alternative? Throw an exception on non-arrays in D12?

Documenting the type change seems straightforward: https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... → , and the signature will change anyway because $is_root_call is going away, though maybe that's not a breaking change.

It would be nice to add a return type at that point, too, if possible.

Context for #10: @larowlan ran into this at https://git.drupalcode.org/project/experience_builder/-/merge_requests/4..., for 🐛 PHPUnit Next Major tests failing Active .

OK, rebased the MR and updated the deprecation version. I also changed the code a bit so that $variables['date'] and $variables['author_name'] are always set, even if empty, so that people debugging Twig templates don't wonder why those variable might be missing.

CR draft: https://www.drupal.org/node/3534020 →
Follow up to add the type declaration: 📌 [PP-1] Add array type declaration to parameters in RendererInterface::render() Active

One thing to consider is whether to add type declarations to $elements in renderRoot(), renderInIsolation(), and renderPlain() as well, but that's probably out of scope for this issue.

Cross-referencing 📌 Move preprocess functions into NodeThemeHooks Active , we should get this in first otherwise it will be a lot more complicated to backport this to 11.2

@berdir I am not sure why line 212 of Renderer.php has /* array */ instead of typehinting the $elements parameter 'array'?

public function render(/* array */&$elements, $is_root_call = FALSE) {

@oily re:

@berdir I am not sure why line 212 of Renderer.php has /* array */ instead of typehinting the $elements parameter 'array'? Not sure it matters. But cannot find any other examples grepping codebase.

We can't add the typehint in a minor version because it would be a backwards compatibility break. Reference: Deprecation process for interface and base class signature changes → . (The second example in point 1 there for adding a typehint illustrates this pattern.).

@larowlan inquired about backporting this to 11.2 despite the deprecation. Since this is a regression in 11.2, a backport is appropriate, but it might be better to create a backport version that does not raise the deprecation (i.e., that just silently returns empty string if the array is empty in Renderer::render(). They'll still start getting the deprecation on 11.3, so it's not like we're hiding their upgrade path from them. But that way we also avoid triggering new deprecation errors in production.

Thoughts?

Oops, well the change can be reverted if needed :)

@xjm re: #15

Thoughts?

I grepped for other typehints using the pattern /* array */ like /* string */. Didn't find any. Is it possible that this is creating unnecessary work? Now just follow line of least resistance and move on to other issues..

@oily, It's an adopted and documented standard as per the reference I linked, so again, out of scope for this issue. Thanks!

I think we should commit this to 11.x as-is.

For the 11.2.x backport, we can do a backport MR here, or possibly just delete that line on commit - it's just the one line with no test coverage changes so it should be a clean deletion. While the deprecation is less disruptive than a fatal error sites/modules that haven't run into this on 11.2 at all yet won't know that and would get a new deprecation added in a patch release that they know nothing about, so might as well just add it in 11.3

@xjm, Yes, some useful reading in the link. I wondered if the 'pattern' was some ad lib idea : ). It being a standard, that seems fine.

@xjm, Yes, some useful reading in the link. I wondered if the 'pattern' was some ad lib idea : ). It being a standard, that seems fine.

xjm → closed merge request !12628

I restored the original "deprecated in 11.3.0" on the 11.x MR, and created an 11.2.x backport MR that is identical aside from removing the deprecation trigger_error().

If anyone can do a quick (but careful, naturally) review of the above two MRs and validate the RTBC, that would be helpful, since I should probably have at least some sort of peer review before merging my own hotfixes to HEAD. 😇 Otherwise we can wait for another committer to come 'round.

The changes in both MR look good to me.

Per https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... → , shouldn't we need to add a phpcs:ignore referencing 📌 [PP-1] Add array type declaration to parameters in RendererInterface::render() Active ? I'm not entirely familiar with the process.

@penyaskito: the parameter and typehint already exist in the docblock, so there's nothing to add there, AFAICT. Not sure how that affects whatever triggers the deprecation warning described in the docs, though.

@penyaskito Shhhhh, I was hoping no one would notice! 😂 It turns out that the phpcs:ignore stuff is actually a total red herring -- it's ignoring a malformatted docblock that decides not to leave blank lines between @todo and @see, and not actually anything to do with the typehint addition itself. @godotislate is also correct that since this is a full parameter addition, it isn't quite the same as the documented example.

I was planning to quietly file a followup for this issue, just to add the expected @todo and @see to the interface to remind us to uncomment the typehint there as well as on Renderer itself. @mondrake and I are ironing out some issues with the excessive phpcs ignores in 🐛 Fix PHPStan arguments.count error Active , and the followup can use what we settle on there. So, okay, I will be responsible now and here's the followup: 📌 [pp-2] Add the @todo and @see for Renderer:render()'s array signature typehint once the docs formatting standard is adjusted Postponed

Normally we would not scope this to a followup, but given the severity of this regression and the current WIP with the docblock annotations, I think the current deprecation and docs are sufficient for this hotfix. Consider this RM signoff on the scope adjustment. 🪄

😀

Oops, I did actually file it.

🤐😶‍🌫️ So today I learnt the process, and thanks for the explanation on why not following it.
I think we are fine merging both MR as they are.

Adding credit for @penyaskito for very rightly pointing out a missing part of the deprecation. 😛

Looking ahead I think we should consider also accepting RenderableInterface as well as array. It will allow us to move towards using render element objects without having to call ::toRenderable

Aka 📌 Slowly, very slowly start OOPifying the render system Needs review

But let's fix the regression first and do that elsewhere

Created follow-up per #33: 📌 [PP-1]Allow RenderableInterface objects to be passed as the $elements parameter in RendererInterface::render* methods Active .

Is there a meta issue about moving to using render elements in the Render API?

larowlan → closed merge request !12134

larowlan → closed merge request !12629

Committed to 11.x
Committed the backport version to 11.2.x
Thanks folks

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

thank larowlan work for me