Updated: Comment #N

Problem/Motivation

If we want to move drupal_render into a class at some point, we need the utility functions it uses to also be in their own class. This will also make the code much easier to test.

Proposed resolution

Move element_property, element_properties, element_child, element_children, element_get_visible_children, and element_set_attribute into a standalone Element class.

Remaining tasks

User interface changes

None

API changes

Deprecate methods above? leaving in wrappers.

CommentFileSizeAuthor
#18 drupal-core-convert-element-functions-2109793-18.patch3.56 KBInternetDevels
#11 element-2109793.patch22.44 KBdawehner
#11 interdiff.txt3.54 KBdawehner
#10 element-2109793-9.patch20.01 KBtim.plunkett
#10 interdiff.txt4.11 KBtim.plunkett
#6 2109793-6.patch16.28 KBdamiankloip
#6 interdiff-2109793-6.txt4.2 KBdamiankloip
#6 interdiff-2109793-6-2.txt1.48 KBdamiankloip
#4 2109793-4.patch15.3 KBdamiankloip
#4 interdiff-2109793-4.txt4.83 KBdamiankloip
#1 2109793.patch11.03 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Title: Convert element_* method in common.inc to a class » Convert element_* methods in common.inc to a class
FileSize
2109793.patch11.03 KB

I have literally run out of time, so I'm just posting how far I got. Unit test is incomplete, plus comments for function in common.inc need work.

Status: Needs review » Needs work

The last submitted patch, 2109793.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

I personally think that it is not time to convert the render api to OOP as just having static methods all over the place is hard.

We should rather avoid to call drupal_render() as much as possible and if we do wrap it in some methods without logic. Given that a missing
test coverage is okay.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
interdiff-2109793-4.txt4.83 KB
2109793-4.patch15.3 KB

I still think this is a good idea, and better than what we have now, often we will want this before the render stage, like form API, see new methods like FormBuilder::elementChildren() for a good example..

This should fix tests and add some more coverage.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/includes/common.inc
@@ -4471,21 +4471,21 @@ function drupal_sort_title($a, $b) {
 function element_property($key) {
...
 function element_properties($element) {
...
 function element_child($key) {

    Let's add @deprecated to all of them 

  2. +++ b/core/lib/Drupal/Core/Render/Element.php
@@ -0,0 +1,162 @@
+   * @param $elements
...
+   * @param $sort
...
+   * @return
...
+   * @param $elements
...
+   * @return
...
+   * @param $element
...
+   * @param $map

    Can we update the logs? 

  3. +++ b/core/tests/Drupal/Tests/Core/Render/ElementTest.php
@@ -0,0 +1,169 @@
+class ElementTest extends UnitTestCase {

    nice!!

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
interdiff-2109793-6-2.txt1.48 KB
interdiff-2109793-6.txt4.2 KB
2109793-6.patch16.28 KB

Great, thanks for the review. Here we go.

Sorry, two inderdiffs. Pure laziness, to not recreate it as one :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Thank you

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

6: 2109793-6.patch queued for re-testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

6: 2109793-6.patch queued for re-testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
Issue tags: +PHPUnit
FileSize
interdiff.txt4.11 KB
element-2109793-9.patch20.01 KB

Let's finish the test coverage and reap the benefits...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt3.54 KB
element-2109793.patch22.44 KB

Is it just me that we are testing things here which actually better belong into a unit test?

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs review » Reviewed & tested by the community

Yep, that is indeed all unit tested now!

RTBC for those changes on top of the original RTBC.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: Convert element_* methods in common.inc to a class » Change notice: Convert element_* methods in common.inc to a class
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed and pushed to 8.x. Thanks!

This'll need a change notice.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

This patch contained a minor API change in that you can now no longer pass strings or objects into element_properties(). Before they were cast to an array, and now Element::properties() type-hints array for the $element parameter. Not disagreeing with the change or anything, but that should be mentioned on the change notice.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: +@deprecated

Let's create a follow-up issue to remove the @deprecated functions entirely.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Here is a quick one: [#2173683]

InternetDevels’s picture

InternetDevels CreditAttribution: InternetDevels commented
Status: Active » Needs review
FileSize
drupal-core-convert-element-functions-2109793-18.patch3.56 KB

Let's create a follow-up issue to remove the @deprecated functions entirely.

Was this issue created? I just checked common.inc and found that there are still several places where element_children used. I've created a patch for this but not sure where post it.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

@InternetDevels
Please use #2181507: Remove the deprecated usages of element_* methods for fixing it.

dermario’s picture

dermario
Primary language German
Location Zürich
CreditAttribution: dermario commented

Hi i just applied that patch (before testbot was ready) and it worked fine for me.

There is one thing i found:

In children method there is element_children() mentioned in a comment:

    if ($sort && $sortable) {
      uasort($children, 'Drupal\Component\Utility\SortArray::sortByWeightProperty');
      // Put the sorted children back into $elements in the correct order, to
      // preserve sorting if the same element is passed through
      // element_children() twice.
      foreach ($children as $key => $child) {
        unset($elements[$key]);

Maybe that needs to be fixed too?

Les Lim’s picture

Les Lim
he/they
Primary language English
CreditAttribution: Les Lim commented
Status: Needs review » Fixed
Issue tags: -Needs change record

I cleaned up dawehner's quick change notice in https://drupal.org/node/2173683 and assigned it to the Drupal core change list.

Re: tstoeckler's point in #15, I wrote up a separate change notice about that API difference and referenced the original change notice within: https://drupal.org/node/2182161

Marking this as fixed; there's a separate issue for removing the deprecated function calls at #2181507: Remove the deprecated usages of element_* methods.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Title: Change notice: Convert element_* methods in common.inc to a class » Convert element_* methods in common.inc to a class
Priority: Major » Normal

Nice, thanks @Les Lim :) Just reverting title and priority.

Status: Fixed » Closed (fixed)

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