ExtensionDiscovery can detect the type of an extension faster, if the 'type' property is declared first.'type' property first.Change all .info.yml files in core to declare the 'type' property first.
While being there, let's also standardize the three required .info.yml file properties to be consistently declared first + in this order:
type: module
core: 8.x
name: Foo
...
This can be scripted. For the sake of retaining potential comments, intentionally not using a YAML parser/dumper.
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | 2209319-nr-bot.txt | 144 bytes | needs-review-queue-bot |
| #26 | 2209319-25.patch | 314.63 KB | andypost |
| ext.info-order.php_.txt | 661 bytes | sun | |
| drupal8.info-type.0.patch | 113.61 KB | sun |
Comments
Comment #1
sunComment #2
dawehnerThis really helps people to not forget the type: $type key, which is quite annoying, at least was in the past.
Do we throw something if it is not defined?
I do agree that core: 8.x should be above the name as it is required like the type.
Comment #3
webchickEw, I really don't like that. :( From a "human" (module author / downloader) perspective, these files make a lot more sense in a logical sequence which more or less how it's done in HEAD, IMO. With the files as they are in the patch, I need to skim past 2 lines of "noise" in every .info file until I find something useful and unique about it.
Assigning to catch to see if he finds the performance reason compelling to do this.
Comment #4
webchickAhem.
Comment #5
catchCan't tell how compelling it is without profiling.
Also I'm wondering how many requests we actually do where we scan these files and don't eventually go on to parse the YAML anyway?
Comment #6
dawehnerON the other hand it really forces people to not forget this two keys. If you miss either core or type you probably don't see the module in admin/modules which is freaking annoying to be honest.
Comment #7
sunThat's really hard to tell — in general, the more our code base gets untangled and decoupled, the less we're able to answer this kind of question.
We'd have to write a pretty heavy call invocation tracking tool that would collect all traces based on all possible call chains. IIRC, XDebug has a related add-on along those lines (but I could be wrong), and of course, the data collection is only the smaller part of the challenge.
However, some hopefully more convincing reasons:
The new
ExtensionDiscoveryscans for all extensions of all types, regardless of which type has been requested.This is what allowed us to remove the much behated
hook_system_theme_info(), which was required to allow (test) modules to ship with themes previously.This aspect increases the possibility of discovering more .info.yml files than files that will be parsed.
ExtensionDiscoveryis used in some places that do not involve YAML parsing at all — e.g., Simpletest uses it to locate all available extensions that could possibly have tests.As @dawehner already mentioned: Consistency.
Declaring the required properties first is pretty much a standard practice in all meta information file standards that I know of; e.g.,
composer.json,bower.json,package.json,jquery.[plugin].json, etc.pp.Likewise, most of these meta information file standards have a
'type'property, too (cf. Composer). Our use of'type'is pretty much identical to that. In case a meta information file standard has a'type', then you normally declare it as one of the first properties.Consistency is a huge help for developers to remember to always declare these properties.
Lastly, duly noting:
If our meta files were .json instead of .yml, then the entire parsing aspect would be obsolete/irrelevant, since parsing JSON is lightning-fast. I really wish we had gone with .json files instead. Not necessarily composer.json, just .json. But I guess that ship has sailed... :-(
Comment #10
dawehnerAdding a related issue
Comment #17
jeroentComment #25
andypoststill makes sense as less files will be read
Comment #26
andypostre-roll
Comment #27
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.