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

+1 for deprecating passing NULL instead

Deprecation seems like a good idea to me too.

For the deprecation

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 #3492722: Update XB to require Drupal 11.2.

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: #3534019: [PP-1] Add array type declaration to $element parameter in RendererInterface::render()

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.

@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.

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 the pattern @Berdir is using.).

@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? I suggest leaving code as it is at so far in this issue, and follow the path of least resistance to close this issue.

Also is there some advantage to this pattern? Does it get picked up by IDE's or something? Or is it intended as a visual placeholder? As a visual placeholder I think may be more a hindrance than a help since AFAIK it is not a recognised pattern (outside Drupal)? In which case it could cause more confusion than enlightenment?

Perhaps I am missing the point? This is a new way of handling typehinting/ deprecations and it is going to be used consistently from now on? In which case it may have been considered in a lot more depth than I can summon here..

@oily, It's an adopted and documented standard as per the reference I linked, so again, out of scope for this issue to change that pattern. In this case, we are unlikely to change that standard because we spent a good deal of time developing it. A similar process is used in other projects. Keep in mind, the comment in the signature is also only a temporary part of the deprecation layer: In the next major version, it will be converted to the hardcoded PHP array typehint you expect. 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, Thanks, some useful reading in the link. I wondered if the 'pattern' was some ad lib idea : ). It being a standard, that seems fine.

#19 +1

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 #3534019: [PP-1] Add array type declaration to $element parameter in RendererInterface::render()? 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 isn't 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 #3520730: Fix PHPStan arguments.count error related to interface argument additions in the next major, and the followup can use what we settle on there. So, okay, I will be responsible now and here's the followup: #3534277: [pp-2] Add the @todo and @see for RendererInterface:render()'s array signature typehint once the docs formatting standard is adjusted

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 #3525331: Reuse element plugins as object wrappers around render arrays

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

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

Is there a meta issue about moving to using render elements in the Render API? Off-topic here, but I was wondering about things like whether render elements should now implement RefinableCacheableDependencyInterface, \Stringable, and whether they should be able to render themselves (a render() method?) - all things maybe to discuss in a meta.

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

thank you larowlan work for me