Remove Drupal\Core\Extension\ExtensionDiscovery::getInfoParser() since it is dead code

This is tricky, a class extending this class could rely on the existence of the function.

Well, I'm just thinking from the perspective of a maintainer, as we have to justify changes later anyway.

This 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.

While 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?

Thank you @mile23!

Thanks @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":

The results of InfoParser are not cached, even though a subsequent info file parsing will yield the identical results.

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.

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.

So 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:

      // Determine extension type from info file.
      $type = FALSE;
      $file = $fileinfo->openFile('r');
      while (!$type && !$file->eof()) {
        preg_match('@^type:\s*(\'|")?(\w+)\1?\s*$@', $file->fgets(), $matches);
        if (isset($matches[2])) {
          $type = $matches[2];
        }
      }
      if (empty($type)) {
        continue;
      }

, so basically we simply don't have the expensive operation anymore.

telling people to just make new InfoParser objects

So 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?

Crossposted with @dawehner; you are too fast. Reading now. :)

Discussed 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!

+1

Subclasses of ExtensionDiscovery should be highly performant.

What 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?

Subclasses

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.

Avoid creating InfoParser objects, because they have a relatively high performance overhead.

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.

Works for me

I 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

Protected methods of a class should be assumed @internal and subject to change unless either the class or method itself are marked with @api. Drupal leaves most internal methods protected rather than private to allow for one-off customized subclasses when needed, but in most cases that "escape hatch" should not be replied upon indefinitely. If no alternative presents itself consider filing a feature request for a more directly supported approach.

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.

So right, this means #21 should be committed to 8.2.x and 8.3.x should have the actual removal?

Nice!

Okay by me too since the method is protected and dead/broken. Thanks! Edit: For 8.3.x that is.

Committed/pushed to 8.3.x, thanks!