Reuse element plugins as object wrappers around render arrays

It looks great idea to get in for D12!

btw it will raise a question of DI for elements as related still)

I converted FieldThirdPartyTestHooks as well so we convert something that isn't a form_alter.

I think a couple of examples outside of tests would be good too.

The functional JS test was failing, in hindsight it was kind of obvious what I missed.

I passed the objects back into the array assuming that toRenderArray would be called as necessary upstream. The idea that @moshe had to add a key property we can set and return the object directly would be nice too.

I got InvalidArgumentException: "field_test_field_formatter_third_party_settings_form" is an invalid render array key. Value should be an array but got a object. in Drupal\Core\Render\Element::children() (line 97 of /var/www/html/core/lib/Drupal/Core/Render/Element.php).
when running through the test manually.

For this particular test: https://git.drupalcode.org/project/drupal/-/merge_requests/12173/diffs?c... fixes it by converting back to the render array.

By request from chx in Slack, adding more credits

One test failure:
Drupal\Tests\system\Functional\Form\ArbitraryRebuildTest::testUserRegistrationRebuild
Behat\Mink\Exception\ElementNotFoundException: Button with id|name|label|value "Rebuild" not found.

Don't think I've seen this one as an intermittent failure.

First real look at the MR. Mostly nits. A few questions of substance. Out of time for now. More later...

@chx: really? Please. The drama is helping no one. phpcs is a tool. It’s not hard to use. You can configure your IDE to get it right. You don’t need to take it so personally and get so worked up and angry over it. Please take some deep breaths, and maybe a day off, but spare us the public agony and tantrum over how having standards is an impediment to velocity.

We’ve been in this together for nearly 2 decades now. You know how much respect I have for you and your abilities. Please know I’m only writing this out of love and care. I invite you to relax and have some perspective.

Thanks,
-Derek

I've added skip rules for a bunch of jobs while we continue to work on architecture, maybe it's worth configuring this so draft MRs don't run these tests by default so that the actual architecture can happen unimpeded, then once the MR is no longer draft we can fix phpcs etc.

I've also added the fix for the fromClass work. I tried running the test locally but for some reason they wouldn't start.

Turns out core seems to ignore these flags, I just set them to allow failure.

Added a test that swaps out a class and added something to attempt to make the standalone wrappers nicer.

There are several failures, we could possibly revert that, but let's think on it a bit.

I updated the IS as well with some next steps, this could use a pass on comments, and we need to finish tests and tidy up the deprecations.

Adding just in case

Thanks @nod_

I removed the skip since it doesn't work anyway here is the issue to track: #3526516: Consider allowing first stage tests to fail when an MR is in draft.

Ok this needs some additional tests, but we are in a good spot for another review.

I think this is ready for review again, we've added tests and addressed most of the feedback.

I also added the deprecation.

Beyond review it might be worth identifying a few more things to convert.

Thank you for the reviews! The latest round of comments should all be addressed. I left a few open that still need confirmation that the answers are acceptable.

Ok I've merged in the changes from the other branch exploration.

I've also rebased on 11.x

High level notes.
We replaced ModernWidget with WidgetInterface.
We added a Generic Element.
We added changetype.

Tagging for myself later.

I've reviewed this a few times and its ready IMO. Great work especially to @ghost of drupal past and @nicxvan

Took a look at this and it wasn't what I was expecting but agree it is a logical first step towards object oriented form/render array.

It see some comments in the code around longer-term plans for D12/D13 of deprecating arrays and using the element plugin instances everywhere.

In light of that I think it would be good to have a parent meta created with some sibling issues for the next steps.

The longer term things I would expect we would move towards would be replacing the @property doc block comments with real properties and using $storage for random overflow (people have gotten used to dumping random things in #somekey, this would give us language level type-checking and possibly memory improvements

Aside from that I think the best course of action is to get this into 11.x for 11.3.x early and unblock the follow up work, but let's articulate what those next steps are first.

Thanks folks.

I will try to update this, I won't have much contribution time this next week though.

We probably need a cr with the new functionality too.

@catch let me know that the longer term plan is to wait for PHP 8.4 and property hooks rather than dedicated properties. Would be good to document that somewhere

I addressed all feedback and created the follow up.

Took a pass at credit.

All feedback addressed. Back to RTBC.

Following the advice of @larowlan, I have checked if this proposal is OK with #3508641: Define form elements from SDC

It looks good to me so far:

• ComponentElement internal logic was not changed, so it will still be able to extend FormElementBase and manipulate some form properties.
• Additions to ElementInterface and their implementation in RenderElementBase are not disturbing and are welcomed

Thanks for the work.

Committed to 11.x and wrote a change record with the new APIs as well as published the existing one

🎉🎉🎉

Great work folks

Came to add that tag and catch already has 🎉

First follow-up #3533842: Use of @internal in RenderElementBase and FormElementBase property attributes yields phpstan errors - getting this in early ++

I think per #3539320: Element plugin object wrappers causing AJAX failures we should roll it back and re-commit it once we have a solution to that issue. I'm not around much this week though so probably can't actually do that.

Reverted per #76 and removed tag/unpublished change record

Thanks for pointing me to this. Terrific.

Excellent work, so far! I'm late to this convo & just reviewed the MR in full. I'd like to help with this & similar efforts.

Please consider taking a look at my old Form Factory Kits project. It does similar (form render array creation/modification). I just released a new version compatible with D11 after doing a broad search to see if the module is still necessary. I had always planned to revise the module functionality to directly enhance core render elements & I agree with your pending changes. I would merely like to ask you to consider the pros/cons of the element "Kits" I created long ago & whether Core should offer similar objects by default. I addressed most Form API use cases, as documented here. If you decide your pending changes are sufficient, that's fine! I'll update/simplify formfactorykits to utilize your new functionality, or close that project if it makes sense to do so :) Eager to see a more powerful Form API! Thanks for all your great work!!

@yesnoio I would love to see this move forward, does your implementation handle the case in https://www.drupal.org/project/drupal/issues/3539320 ?

In a light of the new PHP 8.5 cloning feature and pipes instead of wrappers elements could be readonly classes with some with* methods (wither pattern)

So kinda

$element = $renderArray
    |> ElementWrapper::fromArray(...)
    |> fn($el) => $el->withProperty('title', $entity->label())
    |> fn($el) => $el->withChild('submit', new Submit())
    |> fn($el) => $el->toRenderArray();

Also it could use compatible with PHP 8.3

$button = new SubmitButton(value: 'Save');

// This creates a NEW object, leaving $button untouched.
// No side effects, no reference bugs.
$processedButton = clone $button with [
    'value' => 'Save & Publish',
    'disabled' => true,
];

@nicxvan it avoids that issue. `formfactory` provides functionality for `array $form` import/modification/export (typically done within the `FormInterface::buildForm` method). The module merely provides form array alteration functionality. I expect the issue documented in 3539320 would not occur, since `formfactory` refrains from wrapping form arrays for very long.
If something like formfactorykits were rolled into Core, the same pattern could be repeated (near term): keep form arrays "unwrapped" by default & create a new optional form `FormObjectInterface`. Preparations for more powerful core form objects could continue, if desired. Something like the following could be quickly implemented:

namespace Drupal\Core\Form;
interface FormObjectInterface {
  public function buildFormObject(FormFactoryInterface $form_factory, FormStateInterface $form_state): void;
}

namespace Drupal\Core\Form;
abstract class FormObjectBase implements FormObjectInterface, FormInterface, ContainerInjectionInterface {
    // ...

    public function buildForm(array $form, FormStateInterface $form_state) {
        $form_factory = $this->getFactory($form);
        $this->buildFormObject($form_factory, $form_state);
        return $form_factory->getForm();
    }

    public function validateForm(array &$form, FormStateInterface $form_state) {
        if (method_exists($this, 'validateFormObject')) {
            $form_factory = $this->getFactory($form);
            $this->validateFormObject($form_factory, $form_state);
            return $form_factory->getForm();
        }
    }

    public function submitForm(array &$form, FormStateInterface $form_state) {
        if (method_exists($this, 'submitFormObject')) {
            $form_factory = $this->getFactory($form);
            $this->submitFormObject($form_factory, $form_state);
            return $form_factory->getForm();
        }
    }
}

namespace Drupal\foo\Form;
class FooForm extends FormObjectBase {
    public function buildFormObject(FormFactoryInterface $form_factory, FormStateInterface $form_state) {
        // Create `textfield`, `managed_file` & `submit` elements.
        $form_factory->append($form_factory->kit->textField());
        $form_factory->append($form_factory->kit->file());
        $form_factory->append($form_factory->kit->submit());
    }

    public function submitForm(array &$form, FormStateInterface $form_state) {
       $name = $form_state->getValue('textfield');
       $file = $form_state->getValue('file');
       // @todo save the file with the given name
    }
}

Rebased 3525331-slowly-very-slowly branch and opened MR !13985 to be able to address reported issues

I have just created a separate issue for FormFactoryKits consideration, so this thread can remain focused on the great work you all have already contributed. I plan to create another beta release later today that includes the proposed FormObjectBase class. Discussion about the pros/cons of FormFactoryKits can be had in that & similar issues. Thank you for your consideration & great contributions to the Drupal Community!

so what is the state of issue now? maybe it needs split somehow at least to evaluate formFactorykits

I gave this one some thought a few months ago, but haven't had a chance to investigate or try anything, so here's vaguely what I remember thinking:

• We need to be careful with the reference use, so maybe we don't use references at all
• Without references, conversion between render arrays and objects could involve copying a lot of large arrays. Could this be a memory issue?
• Also, if we're not using references, is there a reason to limit ourselves to the concept of "wrappers"?
• Is there another approach that keeps allows us to maintain BC with render arrays without building a new render system with objects, completely in parallel, relatively from scratch?

I haven't looked at form factory kits at all, but yes, perhaps it deserves a separate issue to look at it.