Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Follow-up to #2305839: Convert hook_element_info() to annotated classes
Will likely be broken down per-module.
Proposed resolution
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#34 | 2311393-hook_element_info-34.patch | 12.35 KB | tim.plunkett |
#31 | 2311393-hook_element_info-31.patch | 12.37 KB | tim.plunkett |
#27 | 2311393-hook_element_info-27.patch | 10.19 KB | tim.plunkett |
#21 | 2311393-hook_element_info-21.patch | 9.94 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettStarting this.
Let's see how the reroll went.
Comment #3
tim.plunkettFixed mistake. I'll probably split this out into dedicated issues.
Comment #6
almaudoh CreditAttribution: almaudoh commented@Tim: This is a really awesome effort!! How do you want to go about this? I'd like to help with patches.
Comment #7
tim.plunkettLet'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.
Comment #8
almaudoh CreditAttribution: almaudoh commentedOpened the following issues:
#2325477: Convert contextual_element_info() into Element classes
#2325479: Convert editor_element_info() to editor_element_info_alter()
#2325485: Convert field_ui_element_info() to Element classes
Will attach patches on those later today.
Comment #9
almaudoh CreditAttribution: almaudoh commentedRaised additional issues::
#2326875: Convert file_element_info() to Element classes
#2326881: Convert filter_element_info() to Element classes
#2326885: Convert language_element_info() to Element classes
#2326891: Convert system_element_info() to Element classes and
#2326895: Convert toolbar_element_info() to Element classes
Comment #10
pwolanin CreditAttribution: pwolanin commentedThis looks like a really substantial API change. I think it should wait until 9 if they are not done by beta.
Comment #11
tim.plunkettAll 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.
Comment #12
tim.plunkettThe patch was moved out, this is just a meta.
Comment #13
tim.plunkettAfter 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.
Comment #14
xjmComment #15
xjmLooks like all the child issues are done?
Comment #16
tim.plunkettComment #17
tim.plunkettWith all docs cleanups.
Comment #18
jhodgdonI 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"?
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:
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.
Comment #19
tim.plunkettI'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.
Comment #20
jhodgdonDo 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.
Comment #21
tim.plunkettRerolled. The nitpick from #18 is on code that has since been deleted.
Comment #22
tim.plunkettComment #26
jhodgdonThat 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.
Comment #27
tim.plunkettReroll.
Comment #30
jhodgdonAccording to testbot, this needs a reroll.
Comment #31
tim.plunkettRerolled.
Comment #32
bill richardson CreditAttribution: bill richardson as a volunteer commentedPatch requires re-roll -- setting to needs work.
Comment #33
almaudoh CreditAttribution: almaudoh commentedComment #34
tim.plunketttheme.api.php moved.
Comment #35
jhodgdonThe patch looks fine... except one nitpick:
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?
Comment #36
Mile23The patch still applies.
At this stage, it should be marked as deprecated for 9.0.0.
Comment #37
Mile23I 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. :-)
Comment #38
tim.plunkettUnfollowing. This issue is embarrassing. It should have been removed months ago.
Comment #39
Mile23Sorry 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.
Comment #40
almaudoh CreditAttribution: almaudoh commentedRe-titling to reflect what the patch is actually doing.
Comment #42
almaudoh CreditAttribution: almaudoh commentedAgain...why can't this be done in 8.0.x
Comment #44
almaudoh CreditAttribution: almaudoh commentedSetting 8.0.x for testbots.
Comment #46
jhodgdonIt 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:
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.
Comment #47
almaudoh CreditAttribution: almaudoh commented#2557331: Update all documentation referencing hook_element_info() to also include RenderElement plugins
Comment #48
tim.plunkettPer recent IRC discussion. #34 still applies.
Comment #49
Mile23This will need some love, as well, if we're deprecating: #2556935: Deprecate hook_element_info() for Drupal 8.0.0
Comment #50
tim.plunkettIf this goes in quick enough, we can close those other issues.
Comment #51
catchWhen 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.
Comment #52
tim.plunkettdrupalcontrib 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()
Comment #53
xjmHm, 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.
Comment #54
catchIn 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.
Comment #55
tim.plunkettYes, that is up to date. I added this issue to it.
Comment #56
xjmAlright, that seems sufficient to me then. Thanks everyone!
Comment #57
catchOK then.
Committed/pushed to 8.0.x, thanks!
Comment #60
larowlanFinal followup #3221798: Remove stale processing and reference to form_type_TYPE_value