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
Drupal\Core\Extension\ExtensionDiscovery::getInfoParser()
documented here: http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Extension/Ex...
a) It's a bad idea since InfoParser doesn't need to be cached by ExtensionDiscovery. (InfoParser keeps a static of all the info it's ever parsed.)
b) Nothing calls it or uses it, afaict.
Proposed resolution
Remove it.
Is a deprecation process needed? No, it's a protected method of a class with no superclasses. If a change notice is needed, all that's required is for callers to say new InfoParser
instead of calling getInfoParser()
.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#27 | 2725839_27.patch | 1.05 KB | Mile23 |
#21 | interdiff.txt | 1.27 KB | Mile23 |
#21 | 2725839_21.patch | 1.07 KB | Mile23 |
#18 | interdiff.txt | 872 bytes | Mile23 |
#18 | 2725839_17.patch | 1.06 KB | Mile23 |
Comments
Comment #2
Mile23The patch.
Comment #3
dawehnerThis is tricky, a class extending this class could rely on the existence of the function.
Comment #4
Mile23Please find it. I couldn't. :-)
Comment #5
dawehnerWell, I'm just thinking from the perspective of a maintainer, as we have to justify changes later anyway.
Comment #6
Mile23If a class is subclassing ExtensionDiscovery in order to get an unused InfoParser, then it is making a mistake.
Comment #7
donquixote CreditAttribution: donquixote as a volunteer commentedThis is why I prefer private over protected any time.
But I agree with the change. I had the same thought when I first saw this method.
Comment #8
Mile23Patch still applies. Restarting test.
Comment #9
dawehnerWhile I totally agree with you, its something which we kind of promised to avoid. What about marking it deprecated and then just drop it in 9.x?
Comment #10
Mile23OK.
Deprecated for removal before 9.0.0, telling people to just make new InfoParser objects.
Comment #11
dawehnerThank you @mile23!
Comment #12
xjmThanks @Mile23 and @dawehner. Leaving at RTBC but adding some notes from reviewing this.
This was added in #2188661: Extension System, Part II: ExtensionDiscovery. From the summary of that issue under "Problem":
So, it seems like it was exactly intend to cache the results of that expensive operation.
However, the fact that it is not used anywhere is indeed suspect. In the original issue, the getter method was used only in
ExtensionDiscovery::process()
. That single usage was removed in #2209293: Remove core compatibility check in ExtensionDiscovery; obsolete with Migrate in core.The equivalent property on the theme handler does seem to be used: ThemeHandler::rebuildThemeData()
...And the theme handler also has the extension discovery, so might even contain two copies of it then?
I'm all for deprecating dead code, but it would be good to clarify here what we do instead to cache the expensive info parsing operation in HEAD, and whether this instance of it has any role in the other work elsewhere in the queue to make extension handling saner, unify theme handling more, etc. Let's link those issues.
Comment #13
dawehnerSo yes, it used to be used in
ExtensionDiscover
but we actually found a way to not have to parse the YML file in the first place, but rather applying some regex on the string:, so basically we simply don't have the expensive operation anymore.
Comment #14
xjmSo actually, this is the part that I'm concerned about. New
InfoParser
objects (edit: once they've done their parsing) are expensive. (They also will bloat the cache so not caching them when they are unneeded is still a reason to do this deprecation.) Maybe we should reconsider what we tell people to do instead though?Comment #15
xjmCrossposted with @dawehner; you are too fast. Reading now. :)
Comment #16
xjmDiscussed with @dawehner. He pointed out that now extension discovery is basically intended to not need to do this parsing. He also said that deprecating/removing this does not interfere with #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList.
So in this instance, I don't think we should actually say anything about creating new
InfoParser()
objects in the deprecation information. Because we don't actually want them to do that; extension discovery is not the right place for it anyway. So instead we can say it's no longer used and that info parsing should no longer be done for extension discovery. Maybe an @see if we really wanted to where to get that information instead or to #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList, but I'm not sure that is necessary.Thanks!
Comment #17
dawehner+1
Comment #18
Mile23InfoParser
has a static cache for all results for anyInfoParser
object: http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Extension/In...But that's a side-effect, and doesn't really apply to this. (And only matters if you've already done the expensive part.)
Howzabout:
Comment #19
donquixote CreditAttribution: donquixote as a volunteer commentedWhat does this mean? That we tell the authors of the subclasses to write fast code please? Or that we predict that those subclasses will be fast?
And "highly performant" is a meaningless term.
Something can be "faster than alternatives we checked" or "faster than what we did previously". Or maybe we can mathematically prove that no alternative exists that is significantly faster for the average use case. But "highly performant" means nothing really. What if someone comes along and makes it 2x as fast? Was the statement wrong then?
Do we even want to encourage subclassing?
ExtensionDiscovery (currently) has some ugly dependencies / an awkward API. Hopefully we will improve this or develop an alternative.
The one subclass I find currently is
Drush\Drupal\ExtensionDiscovery
.All this does is provide a
reset()
method, which should probably exist in the base class.I find this a bit misleading.
Creating multiple
InfoParser
objects, instead of creating a single one and reusing it, can cause performance and memory overhead, because then the same info could be parsed and cached more than once.(But even if we do want to cache an InfoParser instance for reuse, then
ExtensionDiscovery
is the wrong place, as xjm correctly pointed out in #16.)Creating and using one single
InfoParser
object, as opposed to not parsing anything at all, does cause performance and memory overhead, because parsing and remembering is costly.Of course this performance gain is nullified if later on we still parse the info files.
-------------
Btw the parsing is almost a pure function. Almost. Because what if contents of an info file change within a request, or a php cli operation? This is a bit exotic but it could happen. E.g. during a
drush up
. Or in a unit test with an info file in a virtual filesystem.If it was pure, it could be cached statically like a singleton.
Also maybe the
InfoParser
cache should be a decorator, not a subclass.. but that's a different story.------------
Back to the topic at hand:
The message should point out:
- ExtensionDiscovery does not use InfoParser (anymore). It does something faster instead.
- InfoParser caches its data, and it is therefore reasonable to keep the instance for reuse. But ExtensionParser is the wrong place for this.
Comment #20
Mile23Please either suggest something specific to say or write your own patch.
Comment #21
Mile23Added a follow-up #2779311: Move performant info.yml regex to InfoParser
Docblock looks like this:
Comment #22
dawehnerWorks for me
Comment #23
catchI think we could remove this in a minor release if we wanted to - bc policy allows us to do what we like with protected methods outside of base classes:
https://www.drupal.org/core/d8-bc-policy
Given it's dead code in the main class, the likelihood of a subclass calling the method (or for this class, even existing in the first place) seems low.
Comment #24
Mile23OK, if we're allowed to remove it then we should. That would be the patch in #2 which still applies, to both 8.3.x and 8.2.x. Re-running the test there.
Comment #25
dawehnerSo right, this means #21 should be committed to 8.2.x and 8.3.x should have the actual removal?
Comment #26
Mile23@catch in #23 seems to be saying that protected methods are assumed to be @internal. @internal means it could change at any time.
This leads me to think we should remove it for 8.3.x, mostly because I'm not clear on whether we're allowed to remove it in 8.2.x beta. If we are, then the sooner the better.
Comment #27
Mile23So much for 'the sooner the better.'
Anyway, here's the deletion again. The patch applies to both 8.3.x and 8.2.x, so a maintainer can figure out if it's OK to delete against 8.2.x-RC codebase.
@catch in #23 says we're safe to delete this without BC in a minor release.
Comment #28
dawehnerNice!
Comment #29
xjmOkay by me too since the method is protected and dead/broken. Thanks! Edit: For 8.3.x that is.
Comment #31
catchCommitted/pushed to 8.3.x, thanks!
Comment #32
alexpottComment #33
Mile23