Use the builder pattern to make it easier to create render arrays

Edit: nevermind.

I've written some of these for my own purposes in the past. They are quite useful. A general +1 from me.

I agree with @chx, make a template if possible. Also though, #type=>table won't go away that easy and this would make them much easier to build! so +1 from me too

This has some interesting overlap with #2305839: Convert hook_element_info() to annotated classes

I think we should add a setRow() method as well to add a single row to rows. Just a thought.

+++ b/core/lib/Drupal/Core/Url.php
@@ -18,7 +19,7 @@
+class Url implements RenderElementHelperInterface {

What about just RenderableInterface?

Nice idea in the first place!

1. +++ b/core/includes/common.inc
   @@ -3085,6 +3086,9 @@ function drupal_render_page($page) {
   +  if ($elements instanceof RenderElementHelperInterface) {
   +    $elements = $elements->toRenderArray();
   +  }
   

   I think for now we cannot support this line, rather people which create the form have to explicit call it on the render element. The problem is that we lose consistency for example when we do alter the elements.

2. +++ b/core/lib/Drupal/Core/Render/TableElement.php
   @@ -0,0 +1,320 @@
   +  public function toRenderArray() {
   +    return array(
   +      '#attributes' => $this->attributes,
   

   Can we use some reflection on the object to figure this out maybe? This would dramatically improve the DX of writing those helpers

3. +++ b/core/lib/Drupal/Core/Url.php
   @@ -18,7 +19,7 @@
   +class Url implements RenderElementHelperInterface {
   

   This feels wrong given how I consider URL as a value object, not something you directly render but yeah it does have the method as well

All for it, but this will need benchmarks to show how bad the performance hit of adding many many more function calls is ...

So...

For the really big win here... We are already proposing defining the render elements themselves as plugin classes on #2305839: Convert hook_element_info() to annotated classes. Is it possible that we could, instead of having 1 class that defines the render element as a plugin, and another that lets you insert it into a render array with various properties, we could combine all of that into one class? Because they both need to know what the properties are...

Just want to check in here, I confess to not having read much here but #2305839: Convert hook_element_info() to annotated classes has been in for a while now. Is this issue still relevant?

I believe that this issue is properly classified as a feature request, not a task. It is adding a DX feature, right?

Even if it isn't a feature request... If it's a task, it isn't Major or Critical or in one of the "Unfrozen" or "Prioritized" categories. So at this point, unfortunately, I believe it needs to be pushed out to 8.1.x or a later release according to #2144413: Config translation does not support text elements with a format. If you disagree with this conclusion, my suggestion would be to reclassify and add a comment and a piece in the issue summary explaining why it fits into one of the categories of tasks/bugs that need to be done now.

Meanwhile, looking at the patch... I have a few docs suggestions:

a) You definitely need to add something to the Render topic about this functionality, so that developers can discover that it exists:
https://api.drupal.org/api/drupal/core!modules!system!theme.api.php/grou...

b) RenderElementBase has no documentation header at all.

c) RenderElementInterface has barely any documentation. I think it should explain what the class is used for and how to use it. Then the hypothetical section in the theme_render topic can be short and link to this class for further information.

d) RenderElementInterface::toRenderArray() references drupal_render instead of drupal_render()... but actually that sentence is misleading, because actually drupal_render() calls this method itself... That aside, the phrase used for this all over the rest of our documentation is "A render array suitable for drupal_render()" I think?

e) TableElement first-line docs do not end in . and a couple of the member variable docs headers are missing the blank line before the @var.

f) So... At this point reading the patch for TableElement, I'm like , uh.... is this really better than just making a render array directly? And if so, why? I don't really see the benefit of this whole patch. You've added a bunch more classes, just so that you can do this in field.inc:

before:

 $variables['table'] = array(
    '#type' => 'table',
    '#header' => $headers,
    '#rows' => $rows,
    '#attributes' => array(
      'id' => $table_id,
    ),
    '#tabledrag' => array(
       array(
         'action' => 'order',
         'relationship' => 'sibling',
         'group' => $weight_class,
       ),
    ),
  );

after:

  $variables['table'] = TableElement::create()
    ->setHeader($headers)
    ->setRows($rows)
    ->setAttributes(array(
      'id' => $table_id,
    ))
    ->setTableDrag(array(
       array(
         'action' => 'order',
         'relationship' => 'sibling',
         'group' => $weight_class,
       ),
    ));

This is about the same number of lines of code and I don't see that it is all that more understandable. And there's overhead to take this simple object and convert it back into an array.... Why do we want to do this at all? I don't get it.

g) I also don't see why you need all the get() methods on the helper object. They are not ever used and I can't see a reason why they would be.

Ah, I see -- for the convenience of coders, at the probable expense of performance (that needs to be checked, but probably can't really be until we have more of these builder/helpers in place so that you can try building/rendering a typical page with mostly builders and see how much it slows down).

@larowlan are you interested in working on it in a sandbox as a contrib module?

This is an awesome DX improvement. I had a go at what a contrib solution might look like for this problem: https://www.drupal.org/sandbox/sam/2485729

It's a tad rough at this stage, but a great POC. I must admit after spitting out a few images in a matter of seconds, I didn't feel it would be much fun to go back to blindly guessing keys and waiting for an error.

That looks awesome. It can be a handy drush extension in D8.
Perhaps we can define @param type for all the setters by parsing function doc or something.

Thanks for the feedback Jibran. We actually have access to the default values for each variable in the theme definition so we can kind of derive the type from there. Arrays and bools are fairly straightforward and string usually default to NULL.

Awesome, i'm glad someone has started this in contrib.

Great sandbox, please see #2485995: Please do not render early, return render arrays instead for an issue with the current DX, early rendering is a little problem atm. (also for attachments)

Thanks for the feedback. I'll continue the conversation in the issue you created.

If there was a patch generated to provide these wrappers for core is the suffix 'Element' too overloaded given Drupal\Core\Render\Element\Table exists and we are proposing introducing Drupal\Core\Render\TableElement? If they are "builder classes" why not use 'Builder' as a suffix for clarity. IDE integration for class inclusion will also probably match less results with 'builder' as a keyword.

They could probably also do with their own folder, perhaps 'Builder' inside 'core/lib/Drupal/Core/Render' making the final namespace for the table builder class 'Drupal\Core\Render\Builder\TableBuilder'.

Here is a patch which adds functioning builders to core using a modified version of the contrib solution posted above. I think the script that builds the method names needs some work and things like the naming convention and namespace will need agreeing upon, but it's an example of what a patch for this issue could look like.

Testbot

wrong version for a testbot run.

Ah, good catch. Thanks.

Added some extra builders, removed some useless keys.

Just to not give any illusions, but being a feature request this can only go into 8.1.x.

Testing and making patches is fine however - of course :).

Yeah I agree with @fabianx that this is a 8.1.x feature, BUT a really great one.
I'd even call this one of the most influential one, if you are honest.

Good to hear the efforts aren't going to waste. I've been looking over the current progress and methods on each of the builder classes aren't good enough. I can't find a reliable way of determining the keys that are available for each one of the builders. From what I can tell things are documented as a combination of the keys inside getInfo and the preRender method comment.

Docs from 'Range':

<?php
  public function getInfo() {
    $info = parent::getInfo();
    $class = get_class($this);
    return array(
      '#min' => 0,
      '#max' => 100,
      '#pre_render' => array(
        array($class, 'preRenderRange'),
      ),
      '#theme' => 'input__range',
    ) + $info;
  }
?>

<?php
  /**
   * Prepares a #type 'range' render element for input.html.twig.
   *
   * @param array $element
   *   An associative array containing the properties of the element.
   *   Properties used: #title, #value, #description, #min, #max, #attributes,
   *   #step.
   *
   * @return array
   *   The $element with prepared variables ready for input.html.twig.
   */
  public static function preRenderRange($element) {
?>

Compared to docs from HtmlTag:

<?php
  public function getInfo() {
    $class = get_class($this);
    return array(
      '#pre_render' => array(
        array($class, 'preRenderConditionalComments'),
        array($class, 'preRenderHtmlTag'),
      ),
      '#attributes' => array(),
      '#value' => NULL,
    );
  }
?>

<?php
  /**
   * Pre-render callback: Renders a generic HTML tag with attributes into #markup.
   *
   * Note: It is the caller's responsibility to sanitize any input parameters.
   * This callback does not perform sanitization. Despite the result of this
   * pre-render callback being a #markup element, it is not passed through
   * \Drupal\Component\Utility\Xss::filterAdmin(). This is because it is marked
   * safe here, which causes
   * \Drupal\Component\Utility\SafeMarkup::checkAdminXss() to regard it as safe
   * and bypass the call to \Drupal\Component\Utility\Xss::filterAdmin().
   *
   * @param array $element
   *   An associative array containing:
   *   - #tag: The tag name to output. Typical tags added to the HTML HEAD:
   *     - meta: To provide meta information, such as a page refresh.
   *     - link: To refer to stylesheets and other contextual information.
   *     - script: To load JavaScript.
   *     The value of #tag is not escaped or sanitized, so do not pass in user
   *     input.
   *   - #attributes: (optional) An array of HTML attributes to apply to the
   *     tag.
   *   - #value: (optional) A string containing tag content, such as inline
   *     CSS.
   *   - #value_prefix: (optional) A string to prepend to #value, e.g. a CDATA
   *     wrapper prefix.
   *   - #value_suffix: (optional) A string to append to #value, e.g. a CDATA
   *     wrapper suffix.
   *   - #noscript: (optional) If TRUE, the markup (including any prefix or
   *     suffix) will be wrapped in a <noscript> element. (Note that passing
   *     any non-empty value here will add the <noscript> tag.)
   *
   * @return array
   */
  public static function preRenderHtmlTag($element) {
?>

My feelings are that the list of elements and the appropriate methods will need to manually be built. I don't think this will be a big problem in the scheme of things and there is probably a fair bit of code that can be written to make the process of extracting meaningful information from the comments/info methods a lot quicker.

Unless anyone has a recommendation I haven't come across for deriving appropriate methods I'll proceed with getting this underway. The alternative that I can see is a method similar to 'variables' on hook_theme which documents the keys on the element. Not sure how feasible or useful this would be outside this use case.

I have attached a new patch which address a number of issues:

1. The keys which form the basis for the builder methods have been built manually based on the theme/element definitions, templates and documentation.
2. There are now two base classes. A generic one and one for form elements. These encapsulate things standard across these two areas, making it really easy to do standard render API things like setting prefixes and suffixes.
3. The builders generated are ones which developers will and should regularly call out to, instead of a catch-all approach which included a lot of elements which were internal to core modules and would rarely if ever be used outside the context of those core modules.

On a cursory play around with the result, the builders seem more accurate and are improved by the base class.

May as well get it green.

I do really like this, especially for the Table. This looks ripe for refactoring. Can tableselect extend table builder?

1. +++ b/core/lib/Drupal/Core/Render/Builder/ActionsBuilder.php
   @@ -0,0 +1,69 @@
   +  protected $renderable = ['#type' => 'actions'];
   ...
   +  public function setType($value) {
   +    $this->set('type', $value);
   +    return $this;
   

   If each builder sets the renderable array type, do we need a setType? Maybe you can cast between builders and remove that method?

2. +++ b/core/lib/Drupal/Core/Render/Builder/TableselectBuilder.php
   @@ -0,0 +1,121 @@
   +  public function setAttributes($value) {
   +    $this->set('attributes', $value);
   +    return $this;
   +  }
   

   Can these setter methods be added to the base class to avoid the duplication?

Thanks for the feedback.

• I have removed the 'type' from actions builder.
• Attributes shouldn't live on the base because it's not available on every builder.
• I have added inheritance to builders. I have only created relationships between builders which are pre-existing in the \Drupal\Core\Render\Element\ namespace.

I'm interested to hear thoughts on how we could cast between builders of different types. Right now, we could add a key that sets 'type' but then you could have an instance of a builder which represents entirely different output. Perhaps the create method could accept an array of keys so you could:

$table = TableBuilder::create()->setHeader(['header']);
$table_select = TableselectBuilder::create($table->render()); 

No beta evaluation means this needs to go to 8.1.x.

Yeah this has to get postponed to 8.1.x unfortunately, luckily it's a good candidate to be included as a feature in 8.1.x but right now it would likely just block or distract from issues blocking release.

Love this issue though, wish I could favrouite issues:)

Also to respond to #58 certain and most types it would be very tricky to cast between because there may be very few keys. But if you could turn it into an array, change the type and pass the array into say the new builder's constructor, that may work.

Sorry, version was just for the test bot.

Going to bump this for reviews now that we almost have a release and core is slowing down.

Note that core recently added a RenderableInterface, which is for objects that can be dumped to a render array. It's already being used in core. Any additional builder utilities (which I support) should tie into the same system. Ideally, they object can just be stuck directly into the render array and the render system will extract it as needed. (We should verify that works for all objects in core, but I think it does?)

I've implemented RenderableInterface. Not sure how the objects tie into rendering and if they can be used interchangeably with the array equivalent. From what I can see they are only supported as renderable twig variables but this could probably be tied into the Renderer easily enough.

Note that core recently added a RenderableInterface, which is for objects that can be dumped to a render array. It's already being used in core. Any additional builder utilities (which I support) should tie into the same system. Ideally, they object can just be stuck directly into the render array and the render system will extract it as needed. (We should verify that works for all objects in core, but I think it does?)

Well, theoretically this should indeed just works, but to be clear, we need to not try to go too many steps at the same time. Having a 150k patch seems to be one of those.

Can we move that into a contrib module for now so its usable for people that want / need that, then include the patch here step-by-step for 8.1.x?

I have opened #2602368 to look at allowing RenderableInterface to be treated as a renderable array.

I can stick these in a contrib module. The original idea of the contrib module was to provide a build process to automate the creation of the classes but I think it makes sense to keep those two concerns separate for those who just want to leverage the ones in core.

Testbot ahoy.

Can this be factored out any further to reduce some of the duplicate methods and maybe slim the patch down?

This grid may help visualize where the grouping (base classes) could occur
https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...

I don't think we should introduce any base classes unless there is actually a polymorphic relationship in play. It could get very messy, difficult to follow and I think the value might be limited. By it's nature I don't think this patch will be very DRY.

Perhaps I could get on board with a more concrete example?

If all you're doing is avoiding boilerplate code, traits are a much better solution than base classes.

@Crell hoping to get some logical groupings of types elements not at all a goal to avoid boilerplate.

We already have inheritance where it exists for the element definitions. Traits are an option for the grouping but not sure how much value they will bring given we already have a lot of methods on the base builder and the form element base. Happy to look into that in any case.

If the feedback at this stage is optimisation related, can it assumed the approach and structure are validated?

Should we add some tests to theses classes?

My rule of thumb is to test anything new and anything that has broken in the past.

This needs a change record because it is a new API.

The core responsibility of our current render element plugins is to provide render arrays (that's essentially what "element info" has always been in Drupal). As @jhodgdon suggested back in #16, can't we add the additional builder logic to these plugin classes? That will make them easier to find, and will allow us to build on top of the current plugin swappability.

This is an extremely bare example of what I mean. Element plugins' builds are part of their internal configuration (which can be imported and exported verbatim through ConfigurablePluginInterface), and the base class automatically renders this based on elements' default info and optional cacheability metadata. Any child class can choose to expose additional methods to modify the build. This approach lets us convert elements individually, so we can gradually move towards an OO API for render elements, while maintaining compatibility and integration with our current approach.

Since we cannot entirely replace render arrays with objects because of BC (see #2602368: Allow objects that implement RenderableInterface to used interchangeably with render arrays.), here's a method to help configure elements based on render arrays, so we can easily alter render arrays by creating and configuring elements using this method, then export them back to render arrays using ::toRenderable(). I think it would be useful to add ::createInstanceFromRenderable() to ElementInfoManager, which isn't really just an element info manager anymore... We cannot add this method to ElementInfoManagerInterface, because that would break BC.

This will even allow elements to implement PluginFormInterface to provide their own configuration UIs, which could be useful for form building tools.

This was discussed in detail over IRC. Logs are here: http://codepad.org/CToqn2yu

Would appreciate some outside perspectives.

+++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
@@ -126,7 +131,88 @@
+    $this->configuration['build'] = NestedArray::mergeDeep($this->getInfo(), $element);

Does $this->getInfo() give us parity with the builder patch in #64, it was my understanding that "build" array in the builders patch had been manually constructed from a number of places?

I think the IRC discussions touch on it, but it does feel like we'd be making a step towards an OO Render API without actually knowing if it will be a step in the right direction. However, adding the builder classes gives us the DX improvement now and I can see that in the future, regardless of what direction our OO Render API goes in, we'll be able to update those builder classes to return instances of something that make sense in the new Render API.

Also, I think the IDE completion has to be solved, for me, it's a primary part of this issue.

Once we have introduced and released any new classes, we can only alter parameter and return value types as long as those do not violate the Liskov substitution principle. This is especially relevant to class dependencies, because the original builder classes do not use generic factory methods for their dependency injection.

Does $this->getInfo() give us parity with the builder patch in #64, it was my understanding that "build" array in the builders patch had been manually constructed from a number of places?

My patches use ::getInfo() to provide default build arrays, something which any builder solution should be able to do using whatever implementation we see fit. Adding these defaults in the builder gives us more power to build arrays (e.g. we can depend on default property values, check/compare them and act depending on their values).

Why don't we add a render element type that uses #lazy_builder to build a render array out of a proper ViewModelInterface object?
This #lazy_builder callback would be focused on mapping ViewModelInterface to a Drupal render array. Then Drupal\Core\Render\Renderer(Interface) and Drupal\Core\Render\Element\Element(Interface) don't risk becoming too complicated. We can use the same old renderer for now.

(It remains to be seen if we can easily implement full mapping of something resembling Zend\View\ViewModelInterface, but still.)

Because it's not about when we build the render array, but about having a typed API to work with for improved DX. #lazy_builder will not help with that.

Could you clarify how you expect the renderer to get too complicated? Also, how does Zend's ViewModelInterface fit into this issue? We're not all familiar with it.

A #lazy_builder can help with last-minute converting typed data to a render array suitable for the Drupal renderer. Using a separate converter is a nice alternative to each object having knowledge how to convert itself to Drupal render arrays, like with RenderableInterface. To me the separate converter sounds like the simplest solution with regards to our current renderer.
I haven't delved too deep into the patches, I'm sorry if I'm misunderstanding the issue summary.

Zend's ViewModelInterface or similar would be your typed API alternative to 'magic array keys'.
http://framework.zend.com/apidoc/2.4/classes/Zend.View.Model.ModelInterf...
http://framework.zend.com/manual/current/en/modules/zend.view.quick-star...

Pseudo code to compare with the issue summary:

$table_view_model = TableElement::create();
...
$variables['table'] = [
  '#type' => 'view_model',
  '#model' => $table_view_model,
];

That would work and wouldn't be a BC break. It would, however, mean that quite some code may be converted to this, and that dependents would have to increase their minimum requirements. This is in accordance with our policies, but I expect it is a large and disruptive change, especially since both the render array and the object would have to provide the ability to contain children, which begs the question which tree of children would get priority over the other. It sounds similar to #2602368: Allow objects that implement RenderableInterface to used interchangeably with render arrays., but without the BC break.

#79 and #81 should live in its own issue - please - as they are unrelated to this issue - which is about adding the builder pattern to create renderable arrays.

And I really like #90.

Traversing is definitely still a problem.

I also will comment on #2602368: Allow objects that implement RenderableInterface to used interchangeably with render arrays. as I forgot to do so.

@FabianX: Please include arguments supporting your claim that is #79 out of scope. The patch shows a builder pattern, but based on our existing element plugins for future compatibility, rather than by introducing another new concept.

#96: First: Pardon if I came through to harsh. Not meant like that.

Second: Please Fabianx (with lowercase x) - that is important as d.org is not case-insensitive.

Third:

Your patch might enable contrib to do so, but it does not match the issue summary and it feels that it is an implementation detail of Sam's patch.

I have "hijacked" issues before myself (and likely that was not always the best idea - as I had to learn, too - sometimes one is just so enthusiastic :D) and I am not saying it is bad, but I feel that Sam's patch, IS and idea goes into a very specific direction and especially introduces examples, while your patch goes into another and is very generic and abstract.

I of course might be wrong here, but my feeling is still that a new issue would be appropriate as I feel the architectural approach is different, which usually happens when the IS does not match.

With a good IS and supporting arguments your patch might be long in, while this one is still debated. So there might be something for you to win, too.

If things end up similar again, it might be a good idea to merge again or one issue might deprecate the other.

I hope my arguments for the general feeling I got have been easier to understand now.

This concept is something that I think is desperately needed in core.

We've been doing something similar in the Drupal Bootstrap base theme for a few years now, but not as "type" specific as this:
http://cgit.drupalcode.org/bootstrap/tree/src/Utility/Element.php

I've been working on abstracting this into a separate module though, Drupal+:
https://cgit.drupalcode.org/plus/tree/src/Utility/Element.php?h=8.x-4.x

Ultimately, I think core needs better utility classes in general, but I think it's going to have to start from the very basics: #2972143: Create \Drupal\Component\Utility\ArrayObject.

---

Regarding this issue specifically, I think the patches from #79 onwards, while similar, are really a separate issue altogether. I think we should keep this issue focused on the helper classes themselves.

I am going to set to CNW though because I don't think extending from RenderElement is the most appropriate thing to do.

These objects are ultimately just glorified arrays and I think we need some better groundwork (#2972143: Create \Drupal\Component\Utility\ArrayObject) in place first before continuing.

Also, not a huge fan of using the "Builder" suffix nomenclature on all the classes here. Seems a bit overkill, e.g. Table::create() works just fine.

Another thing that this issue should think about is allowing these element classes to be subclassed. Themes (especially base themes) will need to participate in these classes somehow. #2869859: [PP-1] Refactor theme hooks/registry into plugin managers may be able to help in this area eventually.

Re-uploading #64 with some code style fixes, and to see if the testbot still likes it.

Tried to get a patch via gitlab and then apply it manually but it's not applying cleanly. Maybe this is due to how I'm getting the patch?

https://git.drupalcode.org/project/drupal/-/merge_requests/610/diffs.patch

...
patching file core/lib/Drupal/Core/Render/Element/RenderElement.php
patching file core/lib/Drupal/Core/Render/Element/Textarea.php
patching file core/lib/Drupal/Core/Render/Element/AjaxableElementTrait.php
patching file core/lib/Drupal/Core/Render/Element/Textarea.php
can't find file to patch at input line 13130
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|-- 
|GitLab
|
|
|From 66a7d0a60a02141cc0826db63046dd06fca7b7a1 Mon Sep 17 00:00:00 2001
|From: =?UTF-8?q?Ph=C3=A9na=20Proxima?= <adam@phenaproxima.net>
|Date: Thu, 29 Apr 2021 07:39:56 -0400
|Subject: [PATCH 5/7] Fix API conflicts to get tests running
|
|---
| .../Core/Render/Element/ElementAjaxTrait.php  |  2 +-
| .../Render/Element/ElementAttributesTrait.php |  5 ++-
| .../Core/Render/Element/RenderElement.php     | 14 ++++++
| ...eholderFormElement.php => TextElement.php} |  6 ++-
| .../Drupal/Core/Render/Element/Textarea.php   | 45 +++++++++++++------
| 5 files changed, 55 insertions(+), 17 deletions(-)
| rename core/lib/Drupal/Core/Render/Element/{PlaceholderFormElement.php => TextElement.php} (50%)
|
|diff --git a/core/lib/Drupal/Core/Render/Element/ElementAjaxTrait.php b/core/lib/Drupal/Core/Render/Element/ElementAjaxTrait.php
|index aa87f6b7bcb..96b46a7150d 100644
|--- a/core/lib/Drupal/Core/Render/Element/ElementAjaxTrait.php
|+++ b/core/lib/Drupal/Core/Render/Element/ElementAjaxTrait.php
--------------------------
File to patch: 

Idk what the terminology is, but that's a patch formatted for git am. Each commit is a portion of the patch (see the Subject: [PATCH 1/7] Patch #106. in the header)

If you instead use https://git.drupalcode.org/project/drupal/-/merge_requests/610.diff (which is linked at the top of the issue as "plain diff") will have the patch as you usually expect it.

Been through the issue and I see that one of the reason for this is DX but there hasn't been discussion about the actual DX beside "IDE autocomplete". I guess it was a bit too early then, so can we talk a bit about what will be the DX of actual developers? like why are we prefixing all methods with set? especially since there is no get methods. I think we can borrow from JS/jQuery/DOM as far as DX goes.

What is the current API in the patch:

Image::getBuilder()
      ->setPrefix($prefix)
      ->setUri($this->get('url'))
      ->setAlt($this->get('alt'))
      ->setSuffix('</p>')
      ->toRenderable();

 $variables['original']['rendered'] = Image::getBuilder()
    ->setUri($original_path)
    ->setAlt(t('Sample original image'))
    ->setTitle('')
    ->setAttribute('width', $variables['original']['width'])
    ->setAttribute('height', $variables['original']['height'])
    ->setAttribute('style', 'width: ' . $variables['preview']['original']['width'] . 'px; height: ' . $variables['preview']['original']['height'] . 'px;')
    ->toRenderable();

What I would expect:

Image::getBuilder()
      ->prefix($prefix)
      ->uri($this->get('url'))
      ->alt($this->get('alt'))
      ->suffix('</p>')
      ->toRenderable();

 $variables['original']['rendered'] = Image::getBuilder()
    ->uri($original_path)
    ->alt(t('Sample original image'))
    ->title('')
    // There is no autocompletion for random attributes so we could group them
    ->attributes([
      'width', $variables['original']['width'],
      'height', $variables['original']['height'],
      'style', 'width: ' . $variables['preview']['original']['width'] . 'px; height: ' . $variables['preview']['original']['height'] . 'px;',
    ])
    ->toRenderable();

It's starting to look a bit like the DOM, except it's not; so we might need to explain clearly why we didn't go with that (IDE autocomplete, returns an array, not a string, Drupal elements don't map 1:1 to HTML elements, etc.)

About having a list of what goes where, like said in #48 looks like it'll need to be a manual step. As far as human documentation goes once the list is done, since it looks like the DOM, the MDN pages for DOM elements and attributes are a pretty good examples IMO. I like using it at least.

This also makes render array somewhat similar to the DOM… so Drupal Object Model, or DRAMA (Drupal Render Api Model Abstraction) :þ?

Umm I guess there are some existing code that use the set* pattern, still better than render arrays :)

Here is a patch for D10

Added a comment about the phpstan fails in #3259355: Always do a full phpstan analysis on DrupalCI
This will need to be tackled first probably.

Never mind my above comment. Phpstan is right here. Classes mentioned do not exist (anymore).

yeah forgot to add the new files to my patch…

As per the issue summary I remove all the extra code that is not related to the image builder. I do not understand why we add methods to the RenderElement class, it seems completely unrelated. In any case I rerolled the patch with the bare minimum to make the image builder work according to the merge request.

Ok read the logs in #83 which explains why stuff is in renderElement. I'm with Sam152 from 6 years ago personally.

The new MR against 10.x is using code from #64. Not trying to be fancy, just providing IDE auto-completion to create render arrays. It's been years since the work started and #64 would have been a net improvement compared to the still current situation. Solution proposed from #79 feels like too big a step to wrap this issue up in a reasonable timeframe.

I updated the method names to remove all the set prefixes because it's nicer to work with. It's missing almost all the render API stuff, there is just enough to replace #theme => image. We can talk about what should be a trait, in the base class etc. once we agree to go the "easy" way :)

Added type hints

Back to green

I think we should add set prefix to all the setters because it makes it easier to drill down all the setters. Should we add __set as well to BuilderBase?

Then

      $icon = ImageBuilder::create()
        ->uri($icon_path)
        ->width($width)
        ->height($height)
        ->alt($this->getLabel())
        ->toRenderable();

can also be

      $icon = ImageBuilder::create();
      $icon->uri = $icon_path;
      $icon->width = $width;
      $icon->height = $height;
      $icon->alt = $this->getLabel();
      $icon = $icon->toRenderable();

If we don't declare width/height/alt explicitly an rely on __set, we're loosing the IDE autocompletion no? Also how to document the various possibilities?

In the case of dropbutton it allows us to alias properties nicely (assign #dropbutton_type by calling ->type() for example)

In my mind there are nothing else than setters so it's redundant to prefix them. Those are helpers to generate render arrays, altering the render array is still done old school (where you need to know the magic keys names)

It would be good to add testing notes in the issue summary so we know how to manually test this.

For example, the following changed so I assume that checking the pencil icon is showing up would be sufficient for this one.

@@ -84,11 +85,10 @@ function contextual_help($route_name, RouteMatchInterface $route_match) {
       $output .= '<dd>';
       $output .= t('Contextual links for an area on a page are displayed using a contextual links button. There are two ways to make the contextual links button visible:');
       $output .= '<ol>';
-      $sample_picture = [
-        '#theme' => 'image',
-        '#uri' => 'core/misc/icons/bebebe/pencil.svg',
-        '#alt' => t('contextual links button'),
-      ];
+      $sample_picture = ImageBuilder::create()
+        ->uri('core/misc/icons/bebebe/pencil.svg')
+        ->alt(t('contextual links button'))
+        ->toRenderable();

MR doesn't apply and #130

I have re-rolled patch for 10.1.x branch, but as the initially MR is raised against 10.0.x branch It is showing as not mergeable. Anyone with edit access can pls change the target branch to 10.1.x so we can check MR status. Thanks. #130 needs to be addressed.

thanks, updated he branch

reroll looks good

We still need to add typehints and all the fancy php 8.1 stuff that's available now. leaving as NR just to get more eyes on it before sinking too much time here :)

Updated summary examples array() -> []

Attaching related issue for #states builder. #3011365: Introduce FormElementStatesBuilder for #states key of form element render array

Was tagged for tests and change record that I believe still need to happen.

PHP has evolved a lot since this issue was created. Type hints, property promotion, named arguments, etc. So instead of builders we can create value objects. It'll give same DX in modern IDEs.

Builder

$variables['table'] = TableElement::create()
  ->setHeader($headers)
  ->setRows($rows)
  ->setAttributes(['id' => $table_id])
  ->setTableDrag([
    [
      'action' => 'order',
      'relationship' => 'sibling',
      'group' => $weight_class,
    ],
  ],
)->toRenderArray();

Constructor

$variables['table'] = new TableElement(
  header: $headers,
  rows: $rows,
  attributes: ['id' => $table_id],
  tableDrag: [
    [
      'action' => 'order',
      'relationship' => 'sibling',
      'group' => $weight_class,
    ],
  ],
)->toRenderArray();

Overall, this will only mitigate the problem. Dropping render arrays is the only way to fully resolve the issue. I’ve filed a ticket in the ideas queue.

Added change record based on the current issue description.

@chi Re: #143 Good point. But this seems like a major step/ leap forward. I hope this issue be completed as-is as a first step?

@oily The are dozens different render elements in Drupal core. The MR currently covers just a few ones.

if people want to go ahead with this -- and I am not saying they either should or they should not, I really have no advice on that -- then, if someone wanted the opinion of an old ghost then #142 is the way because then it's possible to add RenderElementBase::fromRenderable so that form alter does not need to deal with arrays either. Otherwise, people would need to learn both syntax to be able to debug code. Not to mention how much less code would this require in core. One could even save this gist, run drush scr buildergenerator.php; composer phpcbf $(git diff --name-only core) and then smooth the results. Not saying you should, but, you know, you could. https://i.imgur.com/0Xt8Puu.png https://i.imgur.com/F6e8c1h.png

#2702061-106: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering) and downwards is relevant, but the most likely outcome would be that the number of different elements is reduced, and eventually #theme gets dropped in favour of #type component, but don't really see any reason that this couldn't all be worked on in parallel without explicit dependencies.

I tried #142 but the amount of methods you'd need to add is staggering and at the end of the day since people use arbitrary properties in render arrays you would need to support some sort of set/get anyways so I leaned heavily on that and so #3525331: Reuse element plugins as object wrappers around render arrays is born. If that issue gets accepted this issue might be outdated.

I spent quite some time working on making just 1 conversion, I agree with #153, this is not the way to go about it. #3525331: Reuse element plugins as object wrappers around render arrays is a much nicer solution.

Yes, extending existing Render Elements classes (as proposed in #3525331: Reuse element plugins as object wrappers around render arrays) looks better than adding a Builder class for each Render Element as proposed in the current MR.

Also, when we will replace most (but not all 😉) of Render & Form elements by SDC, all the logic to remove will be contained in a single file.

I think this is good background reading for the other issue, but let's explicitly postpone it on that one.