Updated: Comment #N

Problem/Motivation

We want to clear out common.inc,

Proposed resolution

so lets' move element_info() into some simple service.

Remaining tasks

User interface changes

API changes

@TODO

CommentFileSizeAuthor
#27 element_info-2207743-27.patch11.58 KBdawehner
#22 drupal_2207743_22.patch11.72 KBXano
#19 element_info-2207743-19.patch11.58 KBdawehner
#11 interdiff.txt1.05 KBXano
#11 drupal_2207743_11.patch11.71 KBXano
#9 interdiff.txt6.72 KBXano
#9 drupal_2207743_9.patch11.69 KBXano
#8 element_info-2207743-8.patch7.07 KBdawehner
#6 interdiff.txt726 bytesdawehner
#6 element_info-2207743-6.patch7.1 KBdawehner
#3 interdiff.txt3.95 KBdawehner
#3 element_info-2207743-3.patch6.89 KBdawehner
#1 element-2207743-1.patch3.81 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
FileSize
element-2207743-1.patch3.81 KB

.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented 
  1. +++ b/core/lib/Drupal/Core/Utility/ElementInfo.php
@@ -0,0 +1,68 @@
+namespace Drupal\Core\Utility;

    I'm wondering if this should live in the Drupal\Core\Render namespace or something instead? 

  2. +++ b/core/lib/Drupal/Core/Utility/ElementInfo.php
@@ -0,0 +1,68 @@
+} ¶

    meh

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +PHPUnit
FileSize
element_info-2207743-3.patch6.89 KB
interdiff.txt3.95 KB

meh

Quite meh.

I'm wondering if this should live in the Drupal\Core\Render namespace or something instead?

Good idea!

Also write a basic unit test.

Status: Needs review » Needs work

The last submitted patch, 3: element_info-2207743-3.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented 
  1. +++ b/core/tests/Drupal/Tests/Core/Render/ElementInfoTest.php
@@ -0,0 +1,121 @@
+   * @covers ::__construct

    I guess that is strictly true, seems weird to have @covers for a non-test method though. 

  2. +++ b/core/tests/Drupal/Tests/Core/Render/ElementInfoTest.php
@@ -0,0 +1,121 @@
+} ¶

    Oh, I am sorry. HA

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
element_info-2207743-6.patch7.1 KB
interdiff.txt726 bytes

I guess that is strictly true, seems weird to have @covers for a non-test method though.

True, it is a bit sad as it is the only way to get a 100% test coverage now.

Oh, I am sorry. HA

Don't you get this behavior in your storm instance?

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented 
  1. +++ b/core/lib/Drupal/Core/Render/ElementInfo.php
@@ -0,0 +1,70 @@
+ * Contains \Drupal\Core\Utility\ElementInfo.

    this is a lie:P 

  2. +++ b/core/lib/Drupal/Core/Render/ElementInfo.php
@@ -0,0 +1,70 @@
+ */
+class ElementInfo {

    i guess it would be nice to have an interface:) 

  3. +++ b/core/tests/Drupal/Tests/Core/Render/ElementInfoTest.php
@@ -0,0 +1,129 @@
+   * @covers ::__construct
+   */
+  protected function setUp() {

    oh, thats nice trick!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
element_info-2207743-8.patch7.07 KB

Sure, here is a patch.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
FileSize
drupal_2207743_9.patch11.69 KB
interdiff.txt6.72 KB

Mostly docs fixes, and ElementInfo::buildInfo() is now purer by returning the info rather than setting it itself. Might make it more reusable in the future, and more easily testable.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented 
  1. +++ b/core/includes/common.inc
@@ -4466,25 +4466,12 @@ function drupal_render_cid_create($elements) {
+ * @deprecated As of Drupal 8.0, use \Drupal::service('element_info')->getInfo()
+ *   instead.

    If we are deprecating it, don't we need an issue (novice) for removing that to reference here? 

  2. +++ b/core/tests/Drupal/Tests/Core/Render/ElementInfoTest.php
@@ -0,0 +1,130 @@
+   * @var ElementInfoInterface

    Full namespace needed. 

  3. +++ b/core/tests/Drupal/Tests/Core/Render/ElementInfoTest.php
@@ -0,0 +1,130 @@
+  public function providerTestGetInfo() {

    Nice coverage.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
FileSize
drupal_2207743_11.patch11.71 KB
interdiff.txt1.05 KB

If we are deprecating it, don't we need an issue (novice) for removing that to reference here?

Will do.

Status: Needs review » Needs work

The last submitted patch, 11: drupal_2207743_11.patch, failed testing.

Xano’s picture

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

I talked to Wim Leers about the idea of renaming this to a generic render service, where we can potentially move code like drupal_render() and Element::children() as well, because a service with one method is silly and all these things belong to the same render API. What do you think?

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented

11: drupal_2207743_11.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

It is clear that will need something like a service for drupal_render at the end, but this should be just be responsible for doing the drupal_render logic. On top of that you have services which gives you the element information. I kinda hate to merge a bunch of connected but orthogonal responsiblities.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review

Doesn't look like this needs work at all.

dawehner’s picture

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

Yeah I also disagree with the testbot.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: drupal_2207743_11.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
element_info-2207743-19.patch11.58 KB

Rerolled.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: element_info-2207743-19.patch, failed testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review
FileSize
drupal_2207743_22.patch11.72 KB

Re-roll.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs review » Reviewed & tested by the community

RTBC if tests pass.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +Needs change record

@todo for an issue summary :) therefore needs work

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update, -Needs change record

Updated issue summary and a link to the change notice: https://drupal.org/node/2235461

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: drupal_2207743_22.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Reviewed & tested by the community
FileSize
element_info-2207743-27.patch11.58 KB

Reroll

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Awesome!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Assigned: dawehner » Unassigned

.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Thanks for the change record draft!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed d196e8f and pushed to 8.x. Thanks!

  • Commit d196e8f on 8.x by alexpott: 
    Issue #2207743 by dawehner, Xano: Convert element_info() to a service.

Status: Fixed » Closed (fixed)

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