Slowly, very slowly start OOPifying the render system

dww → credited andypost → .

dww → credited kingdutch → .

dww → credited moshe weitzman → .

It looks great idea to git 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.

dww → credited godotislate → .

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

phpcs won, I am done.

@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: 📌 Consider allowing first stage tests to fail when an MR is in draft. Active

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.

nicxvan → changed the visibility of the branch 3525331-simplify-widget to hidden.

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.

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

nicxvan → closed merge request !12263

nicxvan → credited catch → .

nicxvan → credited donquixote → .

nicxvan → credited phenaproxima → .

nicxvan → credited tim.plunkett → .

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 📌 Define form elements from SDC Active

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.

larowlan → closed merge request !12173

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 🐛 Use of @internal in RenderElementBase property attributes yields phpstan errors Active - getting this in early ++

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

I think per 🐛 Element plugin object wrappers causing AJAX failures Active 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

What can be salvaged? Properties and magic set/get except the elements array is used to initialize, not a reference. Use it more strategic not so general. This here:

    $formObject = $this->elementInfoManager->fromRenderable($form); 
    $widget = $this->elementInfoManager->fromRenderable($element, Widget::class);
    $element = $this->singleElementObject($items, $delta, $widget, $formObject, $form_state)->toRenderable();

Should works fine without references. Inside you are in the "future" Drupal world where you deal with render objects and not render arrays. You can switch in your form alter to such and return back if you so want. But as long as the form and render system is concerned, nothing changes. It's just a helper for more convenient interacting with render arrays.

I am not going to work on this (or indeed, on anything else).