Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
56.32 KB

Starting this.
Let's see how the reroll went.

Status: Needs review » Needs work

The last submitted patch, 1: element-2311393-1.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
579 bytes
56.36 KB

Fixed mistake. I'll probably split this out into dedicated issues.

Status: Needs review » Needs work

The last submitted patch, 3: element-2311393-3.patch, failed testing.

The last submitted patch, 3: element-2311393-3.patch, failed testing.

almaudoh’s picture

@Tim: This is a really awesome effort!! How do you want to go about this? I'd like to help with patches.

tim.plunkett’s picture

Issue tags: +hook_element_info

Let's open one issue per module that implements hook_element_info. I started with #2325049: Convert datetime_element_info() to Element classes.
I'll open more issues tomorrow.

almaudoh’s picture

pwolanin’s picture

Issue tags: +beta deadline

This looks like a really substantial API change. I think it should wait until 9 if they are not done by beta.

tim.plunkett’s picture

All of these could maintain BC if so desired. No reason to wait.

However, all of the conversions are blocked by #2326409: Annotate render element plugins.

tim.plunkett’s picture

Status: Needs work » Active

The patch was moved out, this is just a meta.

tim.plunkett’s picture

After talking with @alexpott, #2326409: Annotate render element plugins is proving to be really hard.
In the meantime, I've rerolled all of the child issues to *not* remove hook_element_info yet, but to do all the procedural-to-method conversions.

xjm’s picture

Issue tags: +Pre-AMS beta sprint
xjm’s picture

Looks like all the child issues are done?

[mandelbrot:d8git | Sun 15:36:20] $ grep -r "_element_info(" *
core/includes/common.inc: *     the defaults for this type of element, defined in hook_element_info(),
core/includes/common.inc: *   An element type as defined by hook_element_info().
core/includes/common.inc: *   An element type as defined by hook_element_info().
core/includes/theme.inc: * - Defaults provided by hook_element_info(), including attached assets.
core/lib/Drupal/Core/Render/Element/MachineName.php:    // were defined in hook_element_info(). Therefore, we apply the defaults here.
core/lib/Drupal/Core/Render/ElementInfoManagerInterface.php:   *   An element type as defined by hook_element_info() or the machine name
core/lib/Drupal/Core/Render/ElementInfoManagerInterface.php:   * @see hook_element_info()
core/modules/contextual/contextual.api.php: * @see contextual_element_info()
core/modules/editor/editor.module: * @see filter_element_info()
core/modules/field_ui/src/OverviewBase.php:   * field_ui_element_info().
core/modules/system/system.api.php:function hook_element_info() {
core/modules/system/system.api.php: * @see hook_element_info()
core/modules/system/theme.api.php: * hook_element_info(), although defining a plugin is preferred.
core/modules/system/theme.api.php: * properties. Look through implementations of hook_element_info() to discover
tim.plunkett’s picture

Status: Active » Needs review
FileSize
4.96 KB
tim.plunkett’s picture

With all docs cleanups.

jhodgdon’s picture

Status: Needs review » Needs work

I thought we still have hook_element_info() though, as well as element plugins? So shouldn't these say something like "defined in hook_element_info() or an element plugin"?

- *     the defaults for this type of element, defined in hook_element_info(),
+ *     the defaults for this type of element, defined by an element plugin,

Oh I see, you're removing this hook. Don't we need a change notice then? And I think hook_element_info_alter() docs will need some attention?

Other nitpicks:

Here the list formatting was mixed up:

- * - Defaults provided by hook_element_info(), including attached assets.
- * Instead, build a render array with a #theme key, and either return the
- * array (where possible) or call drupal_render() to convert it to HTML.
+ * - Defaults provided by \Drupal\Core\Render\ElementInfoManager::getInfo(),
+ * including attached assets. Instead, build a render array with a #theme key,
+ * and either return the array (where possible) or call drupal_render() to
+ * convert it to HTML.

The "including attached assets" part was supposed to be part of the list item. So that should be indented two spaces, and the "Instead..." should start on the next line as it was before this patch.

tim.plunkett’s picture

I'm removing hook_element_info here. We're only keeping hook_element_info_alter.

If someone decides we shouldn't do that, then won't fix this.

jhodgdon’s picture

Do we have a change notice (draft) about this change then? We'll probably have to run it by alexpott/catch, to see if it fits with the beta policies, and we'll need an updated issue summary and a draft change notice before we can do that.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
9.94 KB

Rerolled. The nitpick from #18 is on code that has since been deleted.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Status: Needs review » Needs work

The last submitted patch, 21: 2311393-hook_element_info-21.patch, failed testing.

The last submitted patch, 21: 2311393-hook_element_info-21.patch, failed testing.

Status: Needs work » Needs review
jhodgdon’s picture

That was some kind of test bot glitch...

The docs here look good. And so does the patch, so I am +1 on RTBC.

We do need a change record and beta evaluation though. The patch completely eliminates hook_element_info(), and this definitely needs to be announced.

tim.plunkett’s picture

Reroll.

Status: Needs review » Needs work

The last submitted patch, 27: 2311393-hook_element_info-27.patch, failed testing.

jhodgdon’s picture

Issue tags: +Needs reroll

According to testbot, this needs a reroll.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
12.37 KB

Rerolled.

bill richardson’s picture

Status: Needs review » Needs work

Patch requires re-roll -- setting to needs work.

almaudoh’s picture

Issue tags: +Needs reroll
tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
12.35 KB

theme.api.php moved.

jhodgdon’s picture

The patch looks fine... except one nitpick:

+++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
@@ -688,6 +685,8 @@ public function multistepAjax($form, FormStateInterface $form_state) {
+   * @return array
+   *
    * @see drupal_render()

Not sure what this is returning but if it has a @return it should probably be documented?

So... this issue needs a summary and a change record and a beta evaluation. It's removing a hook. At this point, is that even allowed?

Mile23’s picture

Status: Needs review » Needs work
Issue tags: +@deprecated

The patch still applies.

+++ b/core/lib/Drupal/Core/Render/theme.api.php
@@ -694,32 +691,6 @@ function hook_render_template($template_file, $variables) {
- * @deprecated Use an annotated class instead, see
- *   \Drupal\Core\Render\Element\ElementInterface.

At this stage, it should be marked as deprecated for 9.0.0.

Mile23’s picture

I found this through #2205685: Review @deprecated functions that are not marked "will be removed before Drupl 8.0".

Also it's not really a meta any more. :-)

tim.plunkett’s picture

Unfollowing. This issue is embarrassing. It should have been removed months ago.

Mile23’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed
Related issues: +#2556935: Deprecate hook_element_info() for Drupal 8.0.0

Sorry if it's embarrassing.

I separated the deprecation into another issue: #2556935: Deprecate hook_element_info() for Drupal 8.0.0

This one can be re-rolled for 8.1.x.

almaudoh’s picture

Title: [meta] Convert hook_element_info() to RenderElement and FormElement plugins » Remove hook_element_info() and all references to it

Re-titling to reflect what the patch is actually doing.

Status: Postponed » Needs review
almaudoh’s picture

Again...why can't this be done in 8.0.x

Status: Needs review » Needs work

The last submitted patch, 34: 2311393-hook_element_info-34.patch, failed testing.

almaudoh’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Needs work » Needs review

Setting 8.0.x for testbots.

jhodgdon’s picture

Version: 8.0.x-dev » 9.x-dev

It is too late to remove a hook at this point in the development cycle.

However, we still need parts of the patch... I suppose we need to open yet another issue to update the docs. for instance:

+++ b/core/includes/common.inc
@@ -1196,7 +1196,7 @@ function show(&$element) {
  * Retrieves the default properties for the defined element type.
  *
  * @param $type
- *   An element type as defined by hook_element_info().
+ *   An element type as defined by an element plugin.

This needs to say "hook_element_info() or an element plugin". And there are numerous other spots from the current patch.

So. This issue that is now "Remove hook_element_info()" needs to be 9.x, because that is when the hook will be removed.

But we need an 8.x issue to fix up the docs. If someone wants to create it and make a patch, that would be great.

almaudoh’s picture

But we need an 8.x issue to fix up the docs. If someone wants to create it and make a patch, that would be great.

#2557331: Update all documentation referencing hook_element_info() to also include RenderElement plugins

tim.plunkett’s picture

Version: 9.x-dev » 8.0.x-dev
Status: Needs review » Reviewed & tested by the community

Per recent IRC discussion. #34 still applies.

Mile23’s picture

This will need some love, as well, if we're deprecating: #2556935: Deprecate hook_element_info() for Drupal 8.0.0

tim.plunkett’s picture

Assigned: Unassigned » catch

If this goes in quick enough, we can close those other issues.

catch’s picture

Assigned: catch » xjm

When this got deprecated, it was done so at a time when everything was assumed deprecated for 8.0.0. So.. I think it's OK to still remove it.

I'm not keen on leaving in one-line wrappers, but I do think we should still remove deprecated code which is providing an actual backwards compatibility layer - since that has maintenance costs that single line wrappers don't. Any contrib impact there might still be from removing this has to be weighed against the core impact of leaving it in for 4-5 years.

However:

- are there any actual contrib usages? drupalcontrib.org looks out of date to me. I'd be happier ditching this if there aren't, or if it's only in modules that aren't actually ported at all.

- assigning to @xjm for a second opinion.

tim.plunkett’s picture

drupalcontrib is very out of date, but I went and checked Rules, they no longer have a custom element, so they don't use that hook.
Captcha did, but I posted a patch for them: #2558243: Use a FormElement plugin instead of the deprecated hook_element_info()

xjm’s picture

Hm, tough call. It was marked deprecated before the beta, so the expectation should have been to not rely on it, even if we forgot the words "before 8.0.0". If there were contrib uses, though, that might imply custom module uses as well (which is a reason we can't just remove things whenever the contrib uses get fixed, generally). I'm kind of on the fence about this one.

I found https://www.drupal.org/node/2320115 -- is that complete and correct, and can we attach it to this issue? That way we could toggle the publish on it and send it out the feed again.

catch’s picture

In this case if there are custom uses, they've had more than ten betas to update their code while it was deprecated, so I still think it's fine to remove. That's a one time change vs. 5+ years of maintenance.

tim.plunkett’s picture

Issue tags: -Needs change record

Yes, that is up to date. I added this issue to it.

xjm’s picture

Assigned: xjm » Unassigned

Alright, that seems sufficient to me then. Thanks everyone!

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK then.

Committed/pushed to 8.0.x, thanks!

  • catch committed 9bee458 on 8.0.x
    Issue #2311393 by tim.plunkett: Remove hook_element_info() and all...

Status: Fixed » Closed (fixed)

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

larowlan’s picture