Explore the idea of a BC layer for form and render arrays

I like the idea. Could it be called DeprecatedRenderElement to make it more explicit?

Huge +1 to this! Very slick.

1. +++ b/core/lib/Drupal/Core/Render/MovedRenderElement.php
   @@ -0,0 +1,102 @@
   + * Use this class if you need to modify a render-array and move a render-element
   

   Don't need hyphen for either of these.

2. +++ b/core/lib/Drupal/Core/Render/MovedRenderElement.php
   @@ -0,0 +1,102 @@
   + * $form['some_field'] = new MovedRenderElement($form['some_wrapper]['some_field'], "The render element with key 'some_field' has been moved inside the 'some_wrapper' element. Accessing it direct from 'some_field' is deprecated in 9.1.0 and is removed from 10.0.0. Access the new element inside 'some_wrapper' instead. @see https://drupal.org/node/XXX");
   

   A) Shall we split this into multiline for better readability, especially in example code like this?

   B) s/direct/directly/

3. +++ b/core/lib/Drupal/Core/Render/MovedRenderElement.php
   @@ -0,0 +1,102 @@
   +   *   The new element.
   

   "Reference to the new element."?

4. +++ b/core/lib/Drupal/Core/Render/RemovedRenderElement.php
   @@ -0,0 +1,84 @@
   + * Use this class if you need to modify a render-array and remove a render
   

   No hyphen.

5. +++ b/core/tests/Drupal/Tests/Core/Render/MovedRenderElementTest.php
   @@ -0,0 +1,44 @@
   +    $message = "The render element with key 'some_field' has been moved inside the 'some_wrapper' element. Accessing it direct from 'some_field' is deprecated in 9.1.0 and is removed from 10.0.0. Access the new element inside 'some_wrapper' instead. @see https://drupal.org/node/XXX";
   

   s/direct/directly/ here too...

Thanks!
-Derek

What would be the steps to clean these up once we want to remove the BC layer?

Really interesting idea, while the reasons this is a good idea are obvious, here are some initial thoughts and caveats I can think of:

• Defining where to use this may be subjective and an additional barrier for bugfixes and features getting committed.

  For example, does markup returned from a controller require BC? What about a forms marked @internal? What about render arrays inserted in a hook_form_alter implementation?

  I know these things could be decided on and documented, but I think there'll be a lot of edge cases the policy will struggle to cover.

• They key within the internal form/render array structure is only part of the picture for BC, there'd be other tricker aspects that'd still be hard to provide stable guarantees about.

  Things like, submitted values in form state, structure of the rendered HTML/classes. These can still break custom code, so possibly an exception thrown when these keys are accessed might be preferable.

• Custom code would still need test coverage for deprecations to be caught and trigger, otherwise you're just delaying the break until the next major. Code with tests may already be catching bugs introduced by shifting arrays.
• ArrayAccess and AFAIK all interfaces that mimic arrays never make objects truely behave like arrays.

  Not sure on the specifics, but I believe there are some old discussions around replacing render arrays with objects that dive into some of the edge cases. This may be better than nothing, but it may also be a lot of work to only mitigate some of the BC issues with moving a key.

• Implementing this signals alter hooks as stable APIs. In a lot of scenarios when excessive altering is used, the right choice may be to use a different API and copy parts of the base implementation that apply to your situation.

  For example, if you're significantly altering the structure of a field widget, the right choice may be to implement a new widget. If significantly altering an entity form, the better choice may be a new entity type.

  In my experience, making a new thing is often the better choice vs altering an existing thing. The fact that form/render arrays currently don't have BC guarantees make it clearer for developers what are good and bad architectural decisions, which IMO is an important feature of a framework.

• This approach only works in the case where something is moved 1 for 1, there are a lot of cases where the array structure changes. You'd have no way of proxying access to some old key that no longer exists, into whatever new structure was invented.

Edit: Added an additional point. I suppose the question is: is some render array BC in specific circumstances better than none at all.

I'm not sure if this is the question being asked, but I'm a -1 on changing the policy to make render arrays BC for both practical and technical reasons. But definitely open to be convinced that adding BC for some scenarios is helpful. Maybe in areas where there is significant evidence or examples of folks altering something and it being disruptive.

I've noticed @cilefen does a lot of bug triage around the time of minor releases, I'd be interesting in hearing from them if render array changes have been disruptive at all.

@Sam152 re: #10: Those are definitely good points / concerns / considerations. I was thinking of a few of these, too.

In particular, if you're altering your exposed views forms on certain views to use #states to conditionally hide / show some filters based on the value of other filters, I don't think this solution would be sufficient to preserve BC if we change the structure of the exposed form like we do in #2625136: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements. Even if we had this, that change would still be disruptive for sites doing these sorts of alters. I'm sure we can come up with a lot of potential alterations that wouldn't be helped by this.

That said, it does seem like some BC is better than none.

Re: Exceptions: The TrustedCallback world has plumbing for either triggering deprecations or throwing exceptions. We could do something similar with this, where the default is to trigger a warning, but if you put something in your settings you can get exceptions. We could add an optional final constructor arg that lets specific instances of the class throw exceptions immediately. So phase 1 BC break is to start using this, letting it trigger deprecations. Phase 2 is to start throwing exceptions. Phase 3 is when we completely remove the element.

In terms of policy -- we already have the problem that every bug fix that touches render arrays needs to go through a (mostly subjective) determination of how little/much change is allowed to minimize potential disruption based on severity of the bug. Yes, having this tool would give us more reasons to have subjective debates about scope and disruption, but hopefully it will also let us move forward on more such bug fixes instead of just being stuck in limbo for months/years because There's Never A Good Time For A Disruptive Change(tm) in Drupal anymore.

@dww Interesting ideas and approach, sounds like some very useful avenues to explore.

Out of interest though, as someone who spent a lot of time working on that patch, would you prefer introducing an additional tool that signals disruption, potentially resolves some issues and not others, becomes an additional avenue for blockers and requires additional future maintenance and cognitive load while reading code OR a clarification of the render array BC policy signed off by committers that explicitly allows render arrays to change in order to fix bugs.

This doc (https://www.drupal.org/core/d8-frontend-bc-policy) already says:

We will change these structures where necessary to make usability improvements or add features in minor versions. This means alter hook implementations may need to be updated.

I realise users who write custom code are important and shouldn't be inconvenienced, but fixing long standing bugs and the sanity of the developers fixing them are also important.

I'd much rather have fixed that major accessibility bug 6 months ago. I keep trying to get it fixed before the next round of RCs (8.9.0-rc1 and 9.0.0-rc1), but the release managers object and want to wait for 9.1.x (even though it seems like it should be allowed based on our stated policy that you quoted). Even in 9.1.x, they're likely going to wring their hands about the potential disruption and probably raise the objection: "we need a formal way to deprecate stuff like this." That's why I'm +1 to this, b/c I hope it'll help reduce friction for fixing bugs. If we could reduce that friction without this change, I'd be happier still, but I don't think that's realistic. Now that Drupal is being marketed as having a "continuous upgrade path", even major version changes are supposed to be "non-disruptive". /shrug

Maybe the policy needs to be reiterated on alter docblocks?

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.