Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
drupal_parse_info_format is ugly and nobody understands it very well, so let's replace it with the Symfony YAML parser. We already have the yaml component in core and yaml is a lot more friendly than our info file format.
What's left
Nothing! Commit it!
Suggested commit message:
Issue 1793074 by cweagans, chx, sun: Convert .info files to YAML
Comment | File | Size | Author |
---|---|---|---|
#183 | 1793074.fix-info.183.patch | 1.07 KB | alexpott |
#181 | _fix-info.patch | 1.28 KB | sun |
#176 | 1793074_157-convert-info-to-yaml.patch | 248.1 KB | chx |
#164 | before.txt | 617.33 KB | cweagans |
#164 | after.txt | 635.87 KB | cweagans |
Comments
Comment #1
cweagansOkay, I've attached two patches and a PHP script.
1793074_common.inc.patch is just the changes to common.inc
1793074_1-info_to_yaml.patch is all changes
convert-info-yml.php.txt is the script that I ran from the command line to convert our info file format to YAML (automated load -> process -> dump script). Put this in your Drupal root, and run it with 'php convert-info-yml.php' (don't forget to rename it).
Tests will likely fail, as this is incomplete, but I'm going to set to needs review anyways to see how many.
Here's what's left to do:
* Our previous file format allowed us to stick arbitrary PHP constants in .info files (such as VERSION). We need to decide if we want to keep this functionality, and if so, how we want to implement it. chx suggested working on the Yaml component upstream to allow user-defined value types. It is my opinion that this functionality is not worth the pain involved in getting it working again, and the only reason we support it is because parse_ini_file() supports it. The current parser replaced parse_ini_file() and we decided to maintain backwards compatibility (thanks for digging up that info, chx!). IMO, we don't need to support arbitrary PHP constants, but supporting VERSION is quite handy, so my suggestion is just to check to see if there is a top-level key (it can't be buried somewhere) called version. If the value is VERSION, we substitute in the value from the PHP constant and call it good. I don't think we need any other functionality there, but I could be wrong.
* Make the tests pass. I know that there's at least one test that is testing the old functionality. We can likely just delete that test, since the YAML component comes with a fair number of unit tests of its own.
Right now, Drupal will install with this patch applied, which really surprised me. But if you go to /admin/modules, the version column just reads VERSION for all modules (for reasons mentioned above). Same with /admin/themes.
Comment #3
joachim CreditAttribution: joachim commentedMy 2p is that this makes arrays in .info files easier to read and write.
Comment #4
cweagansYou think that's good, check out bartik.info (especially regions and stylesheets) :)
Comment #5
cweagansTestbot error is http://paste2.org/p/2260152. I'm not sure what the problem is. :(
Comment #6
cweagansTrying something else.
Comment #8
kika CreditAttribution: kika commentedmodule.info or module.info.yml ?
Comment #9
cweagansRight now, it's just .info. If we want to change it to .yml, we can, but that needs more discussion. It doesn't really matter that much to me, so I'm okay with either way.
Comment #10
cweagansNew patch to account for ban.module. Also added substitution for the VERSION constant in common.inc.
Comment #11
cweagansComment #13
joachim CreditAttribution: joachim commentedThis magic should be mentioned in the function docs.
Comment #14
cweagansNew patch. Tests are passing locally, let's see what the testbot says.
I added the commend that joachim asked for, and I also changed the YAML dumper indentation to 2 spaces to be more in line with our coding standards.
Comment #15
cweagansComment #17
cweagansForgot the common_test_info.txt changes.
Comment #19
cweagansOne more. This should fix InfoFileParserUnitTest
Comment #21
cweagansI see the problem now. The test was still calling drupal_parse_info_format(), which doesn't exist anymore. Changed it to just use Yaml::parse(). This may sort of duplicate some of the Yaml component unit tests, but having our own test will ensure that Yaml parsing continues to work for our purposes.
Comment #23
cweagansFixed multiline YAML syntax
Comment #25
cweagansAnd another. I've left out the multiline test, because who really uses multiline strings in their .info file? The functionality is there if people want to try to use it, but we don't use it anywhere in core, so I don't really care if it works or not.
Comment #26
cweagans.
Comment #27
kika CreditAttribution: kika commentedAFAIR support for multiline strings were one of the reasons why Steven implemented drupal_parse_info_format()
Comment #28
kika CreditAttribution: kika commentedfixing tag
Comment #29
cweagansRe #27: The Yaml parser supports multiline strings. We just don't use them anywhere in core. There's a unit test that ships with the Yaml parser that ensures this functionality works. InfoFileParserUnitTest has more or less become a test to ensure that the specific functionality that we need in core continues to work. I have considered just removing InfoFileParserUnitTest altogether, since the Yaml component comes with so many tests.
I wish there was some integration between SimpleTest and all of the PHPunit tests that our 3rd part code ships with. That would make this entire point moot.
Comment #30
RobW CreditAttribution: RobW commented<bikeshed>
.info.yml would help me (general to front-end drupal dev) keep better track of what exactly the file is, and help my code editor open it with appropriate highlighting.</bikeshed>
Comment #31
cweagansI don't really have any preference on the naming, but I imagine that is going to break things. I think my preference for this patch is to just change the syntax to YAML and then address renaming .info files to .info.yml or just .yml in a followup issue, but if we need to handle it here, we can.
Comment #32
Lars Toomre CreditAttribution: Lars Toomre commentedI think that this issue should just make sure that converting *.info files to yml format works. Whatever those files are named in the end should be left to a follow-up issue. Something that perhaps might want to be addressed in that follow-up issue is how we deal with changes for a different version of yml for the module.
Comment #33
chx CreditAttribution: chx commentedNote: the current info parser uses get_defined_constants which is a memory hog if I ever saw one.
Comment #34
tstoecklerThese changes seem unrelated?!
This is a super weird place to put such extensive and useful documentation, but it feels wrong to remove it. I'd just keep it for now.
Apart from that, looks really really great. I would personally like to see $modulename.yml instead of $modulename.info, but since that would require some changes to the module discovery system, you're probably right to leave that to a follow-up.
Comment #35
cweagansRe seemingly unrelated changes: I removed the examples on those lines because they are not easily represented as one-liners in YAML. This is a good thing, IMO. I can put it back, but I'm not sure how we should handle multi-line YAML snippets in comments.
The second change is the result of my automatic conversion script. It must have stripped the comments. I'm inclined to put that somewhere else, but I don't really know where, so I guess I'll just leave it where it is. I'll reroll later tonight.
Comment #36
sunDoesn't seem to have been mentioned yet, so here we go:
1) Yaml::Parse() actually supports parsing of PHP snippets within YAML built-in. However, we do not want to enable that and we do not want to use that.
2) Symfony 1's sfConfig module as well as Symfony 2's DependencyInjection component actually support(ed) a special
%CONSTANT%
notion for external parameters (also: Look ma! Drupal made it into Symfony docs! ;)) — I wasn't able to locate the code that actually parses this though. In general, Symfony rather seems to solve this via (quite heavy) post-processing of configuration data, turning the entire [file] config resource into a tree builder object, similar to a DOMDocument...3) Yaml has built-in support for serialized PHP objects, in case that is of any help. (I'm exploring to use this facility for translatable strings.)
Post-processing seems to make most sense from my perspective. The
%SOME_CONSTANT_NAME%
syntax seems worth to adopt. We're free to decide to limit support for constants to top-level properties. Which, in turn, would make the post-processing pretty straightforward, I think.Comment #37
cweagansSure, there are ways to make it work, but my argument is that it's not worth it. We only use VERSION in core, and my patch above takes care of that one special case. I'm not even sure why we supported arbitrary constants in the first place (other than for backwards compatibility with parse_ini_file() ) - I really can't think of a good use case for them, other than VERSION.
Comment #38
tstoecklerUsing the %CONSTANT_NAME% syntax, we could avoid the get_defined_constants() (see #33), by using constant() directly.
With array_walk_recursive(), I don't think we even to limit this to first-level properties.
I'm not sure either that is actually worth it, though. I think simply supporting VERSION for now makes sense. I wouldn't know of any other use-case that needs constants in info files. Even VERSION is completely unnecessary code-wise, it only exists to make rolling new releases easier. I have never understand why we don't maintain a release.sh, which finds all modules, and updates their version string, but that is another issue. I generally don't see a case for constants in info files.
Comment #39
tenken CreditAttribution: tenken commentedI have a silly question, or maybe this has been addressed already. Any module I get off of Drupal.org has the following comment in it somewhere (usually):
; Information added by drupal.org packaging script on 2012-04-19
My understanding is that YAML doesn't support comments. How will such documentation remain in .info files from D.o or documenting monkeys in their private module .info files.
Comment #40
cweagans@tenken, Actually, that's not a silly question at all. You bring up a good point. The packaging system will need to be updated to use a
#
instead of a;
for Drupal 8 modules.The comment, then, would read:
# Information added by drupal.org packaging script on 2012-04-19
(YAML does support comments - a leading hash sign. I don't think it supports block comments, but neither does our current parser)
Comment #41
joachim CreditAttribution: joachim commented> The packaging system will need to be updated to use a # instead of a ; for Drupal 8 modules.
That's something to mention in the change notice. Which we'll need :)
Comment #42
sunWe should always use the non-inline syntax.
If you converted the .info files through a script, you can borrow from Config:
http://drupalcode.org/project/drupal.git/blob/HEAD:/core/lib/Drupal/Core...
The comment added by the packaging script is actually not the only problem with that.
There are various pieces in the d.o infrastructure that need to be adjusted and orchestrated for the new YAML format, at least one of them being http://drupal.org/project/project_dependency - but most probably also Drush and other stuff...
And, all of the infrastructure stuff still needs to work for earlier versions of Drupal core.
I therefore think it would be best to actually rename .info files into .yml files, so that these external tools and scripts can properly identify what they're dealing with (they'd have to open each file and perform some heavy syntax checks in order to figure it out otherwise).
...and doing that inherently leads to the question whether we don't want to use a single standard filename for each module (e.g.,
module.yml
) instead of the module name......and considering that inherently leads to the question whether we want to have
module.yml
, or perhaps not rathercomposer.json
...(I know... I'm terribly sorry :-/)
Comment #43
sunRegarding the last bits, see #1398772: Replace .info.yml with composer.json for extensions
Comment #44
Crell CreditAttribution: Crell commentedI'm glad sun mentioned it before I did. :-) I have to say I'm very skeptical about this change, since I still feel that switching to Composer for module definitions and dependencies makes far more sense. Using YAML for it just swaps one proprietary parser for a non-proprietary parser, but we're still on our own to make use of the data. Composer would give us functionality, too, and is far more promising. Composer uses json instead of YAML (a different non-proprietary format).
I'd rather not have to change the file format twice. I'd rather see the energy here going into making Composer work for modules, so we can leverage that functionality, too.
Comment #45
cweagansI would like to mirror this sentiment: http://drupal.org/node/1398772#comment-5527088
That's where YAML comes in, IMO.
Comment #46
Crell CreditAttribution: Crell commentedMoving to Composer for anything Composer is capable of handling, and leaving the rest to a new YAML file, that I could get behind. (However, Composer can handle a lot, and has a part of its namespace reserved for additions...)
Comment #47
cweagansI am personally disinclined to use Composer at all, especially for a complete replacement for our .info files. JSON syntax is rigid and unfriendly, and I could see it being especially troublesome for theme developers. JSON is also very verbose for what we're doing in our .info files. I really think that YAML is a better fit.
That said, I can see where Composer could come in handy and I'd like to propose that we simply auto-generate composer.json alongside modulename.info in our packaging system.
I dunno...if they're just using drupal_parse_info_file, then there's not a whole lot of reason for those systems to change. They'll still get the same data back. But that's all beside the point - we can tackle that after this issue.
That is a separate issue. External tools can and should be using drupal_parse_info_file, which will return the same data as before, regardless of the filename.
IMO, that's quite a leap. And also a separate issue.
This issue really should only be about changing the .info file syntax to YAML. I'm disinclined to wait on the composer.json issue being resolved. It's been open since January with no patch. This issue has a patch and all the tests pass. There are some issues (noted above), but I'm going to get them cleaned up in the next day or two. My concern is that, if the composer.json issue doesn't pan out, then we'll still be stuck with the syntax we have now, and I definitely want to avoid that. It's awful.
Writing a conversion script is not difficult. The first pass on the one above took me all of about 15 minutes. If we decide that we want to switch to composer.json later for whatever reason, it's not a difficult problem to solve.
Comment #48
sunPutting YAML into an .info file is like putting JSON into an .xml file. I don't see that as a viable option.
Moving from .info to .yml automatically resolves the issue on the d.o packaging script injecting INI/.info style comments into each .info file. It will have to be patched to do the same for .yml files, but at minimum it won't make the fake-.info files syntactically incorrect, since it cannot add any comment to an .info file that does not exist.
Once you do that change (and IMO we must), discovery becomes a problem, since we're applying and implying a poor uniqueness aspect to some .info files (e.g., themes) currently. Hence, you very quickly end up on the topic of needing a unique filename for discovery.
Comment #49
cweagansExcept that we're not putting json into an xml file. We're putting info/metadata into an info file. It just happens to be formatted as YAML, just as our current info files happen to be formatted like a .ini file. I really think that renaming *.info to *.yml is out of scope for this issue. The goal here is to get rid of drupal_parse_info_format and I've done that.
Comment #50
joachim CreditAttribution: joachim commented> ...and considering that inherently leads to the question whether we want to have module.yml, or perhaps not rather composer.json...
That would make it a real pain when working with a project containing lots of modules and thus lots of info files -- your text editor would have several tabs with identical names.
Comment #51
patcon CreditAttribution: patcon commented@joachim
I respectfully suggest you overestimate the importance of this. Every other community using a package manager (that I know of) deals with this. Every ruby project (gem or otherwise) has a
Gemfile
, every node project has apackages.json
, etc :) < /bikeshed>EDIT: I'm with Crell on the larger issue.
Comment #52
Crell CreditAttribution: Crell commentedHaving a file containing YAML that is called .info breaks syntax highlighting and code formatting in IDEs. That's case-closed for me. The .info extension is dead.
Composer *can* use alternate file formats than JSON. I spoke with Seladek (one of the maintainers) about it 2 hours ago. The fact that no one else who is using Composer has done so (AFAIK) should be a sign that JSON is not the evil clumsy format some people make it out to be.
The reason the Composer issue has been held up is that people are squeamish about the non-project data in info files, and a few are squeamish about raising the barrier to entry for devs (which I think is a red herring). Using YAML for the non-package files solves the first, and that I'm OK with. For the second, we need to just get over ourselves. :-)
So, I suppose I'm OK with this as a stopgap, but Composer for the package data is where we really ought to be driving toward and should be the next issue addressed after this one.
Comment #53
Lars Toomre CreditAttribution: Lars Toomre commentedA big +1 to #52 from me. If it breaks IDEs and it is unclear what is in an info file, I believe we have to use a different extension.
As I understand from scanning the issue, it appears that we are not losing the ability to parse *.info files in the traditional format (at least yet). So let's make it simple to explain and to see just by looking at the extension. It will definitely help the developer experience.
Comment #54
cweagansI already have to configure my IDE to understand .info. I tell it plain text. Now I'd have to tell it to use YAML.
I don't know of any other projects that would be using composer for themes, though. That's my big concern - making sure that it's easy enough for theme developers. YAML doesn't enforce brackets or anything, and it's very natural feeling to write:
rather than
@#53: Not sure what you're +1'ing - renaming everything to .yml ? Also, the patch above certainly does remove the ability to parse our current .info format. It's completely replaced by the YAML parser.
Comment #55
chx CreditAttribution: chx commentedIDEs can add file extensions. Few IDE recognizes .module as PHP and yet we live. That's a non-issue.
Using Composer for this sounds impossible to me unless someone shows up with at least partially working code for it.
So far everyone was for replacing custom Drupal things with Symfony things now that Cameron wants to do the same with .info parser to YML parser we are all over not? I am sick of that double standard and you know that.
Comment #56
pounardI'm tired of reconfiguring every IDE I have to use in order for it to understand Drupal stupid file naming logic. PHP is PHP, let's call a cat a cat and rename all our PHP files dot php. I'm with Crell, renaming YAML files .info is a no go and would be yet another huge Drupal WTF.
Comment #57
joachim CreditAttribution: joachim commented> IDEs can add file extensions
And that's a one-off configuration. How many different IDEs do you use?
How is that worse DX than identical filenames kicking around, which is a continual thing?
Comment #58
a.ross CreditAttribution: a.ross commentedIMO, having the "right" file extension is just a convenience, nothing more. A welcome one, but I think it's being rather exaggerated.
There is another issue that may be interesting in the context of this one also, so I'm cross-linking it: #1545684: Separate version data from the .info file
Comment #59
bhauff CreditAttribution: bhauff commentedI vote for YAML as the info file format. It is commonly used in Ruby on Rails and other projects for this kind of functionality. JSON is not that bad, but it is more verbose for this purpose. YAML was designed to be used by humans; JSON is better than XML for use by humans, but not as friendly as YAML.
Comment #60
Crell CreditAttribution: Crell commentedTo restate my earlier point more directly:
The package parts of info files should move to Composer, because that is the Right Tool For The Job(tm). It uses JSON for its config. Big fat hairy deal. I don't like PSR-0 either but we adopted it because it was the right thing to do. Blocking composer for boo-hoo JSON reasons is pointless bikeshedding.
The non-package parts of info files make sense to move from proprietary info files to standard YAML files.
Logistically, it makes sense to kill .info first and replace it with YAML (this issue), then move forward with Composer. That's more work, but the Composer stuff is stalled and this isn't. So I'm +1 on this patch with the caveat of "and then move some of it to Composer afterward".
Comment #61
RobLoachFully agree utilizing Composer for the package information would be huge, but what's blocked on the Composer front?
Comment #62
cweagansWell, the issue has been open since January and there have been no patches posted, so there's one thing.
Comment #63
cweagansLet's try this. I'm *guessing* that this won't pass, but I'm giving it a try anyways. Maybe I'll be pleasantly surprised.
This patch attempts to use .yml as the extension for metadata files.
Comment #65
star-szrRe-adding change notification tag.
Comment #66
jwilson3Added side benefit: assuming this patch lands, it would fix the bug in #120597: .info domains access denied for D8. (Still no luck for d7 there tho).
Comment #67
cweagansOkay. Let's give this one a try.
- Convert info file format to YAML syntax (including the leading "---")
- Rename all .info files to .yml
- Change all .info discovery functions to account for .yml extension
- Make changes to common.inc (remove drupal_parse_info_format() and fix drupal_parse_info_file())
Tagging because this is a PITA to reroll.
Comment #69
kika CreditAttribution: kika commentedJust curious, what's the reasoning behind leading "---" ?
Comment #70
cweaganshttp://www.yaml.org/spec/1.2/spec.html#id2760395
Not completely necessary, but since we seem to care about how editors deal with the yaml files, I included it because some editor (don't remember which one) only highlights YAML correctly if it's in the context of a document (rather than the directives section). It's a poor way to code a syntax highlighter, but it happens.
So to answer your question: "editor compatibility".
Comment #71
joachim CreditAttribution: joachim commentedI really don't want to bikeshed, but *.info.yaml would keep the bit that tells us what the file does.
Comment #72
cweagansIMO, filename != documentation. composer.json doesn't really describe what the file does either. imo, modulename.yml is quite sufficient.
This is also cnw because of test failures and exceptions in #67
Comment #73
joachim CreditAttribution: joachim commented...which would be why we have foo.module, foo.admin.inc, foo.install, foo.views.inc and so on :p
I do think we would be losing something here: the contents of a module folder become less easy to understand. Also, what happens if a module wants to contain a yml file that's not a module info file?
Comment #74
pounardAnd how about a systemic "modules.yml" or "package.yml" in the module folder, describing all known modules in the current folder.
For example, consider this package:
Accompagnied with an index package.yml file such as:
I'm not saying this would über cool, I would like composer.json alternative very much instead because it has all we need, including defining dependencies with versions. I just like to propose radically different alternative: less yml files, normalized name, package index in one file. Easier to read for human beings, easier to find because we a fixed name, etc...
EDIT: As fun as it sounds, core could provide only one package.yml file! And even funier, the update module could extract the root package.yml file, read it and have metadata about the package without extracting it for example.
Re-EDIT: Added minor details into the pseudo package.yml file upper.
Comment #75
sun1) It's not clear to me how the patch protects against false-positives. AFAICS, Drupal will detect and treat any .yml file that happens to match DRUPAL_PHP_FUNCTION_PATTERN as a module/theme/profile extension metadata file. A protection against false-positives is mandatory, IMHO. If you have a better idea than the static filename I mentioned earlier, then I'm happy to learn about it.
2) The document separator '---' is optional and should not be included. We don't put that into config files either. An editor that doesn't recognize YAML by the .yml extension is broken; no need to care for that.
Comment #76
Crell CreditAttribution: Crell commentedpounard: Package and dependency management should move over to Composer after this patch; let's not fret about that here. Let's stay focused on the 1:1 conversion of info files to YAML.
sun's concern about false positives is valid, though, as there will be lots of CMI files that just so happen to conform to function name rules, and reading those as module definitions would be, er, bad. :-)
Comment #77
RobLoachWhat happens with Drupal.org packaging projects in a different format than .info? Is that just that Project Dependency project that writes the .info/yaml/composer.json file? Or would that be Project module?
As a side-note, I put together a small guide on how to translate a .info file to a composer.json.
Comment #78
cweagans@sun, @crell: I see now. Okay, so given the false positive concerns, how about we use .info.yml for the file extension (as joachim suggested)? This makes the filename more descriptive and should prevent false positives.
Comment #79
sunThat sounds like an acceptable stop-gap/workaround fix, yes. However, I wouldn't want to see that in the final D8 release.
Comment #80
patcon CreditAttribution: patcon commentedWhat about just choosing a static filename like
module-info.yml
and having it consistent everywhere? Why is the focus on usingmodulename[.info].yml
and then having a way to quickly weed out imposters? Isn't the first approach simpler?Comment #81
cweagans@#80, that's already been addressed. It would suck to have a bunch of tabs open called module-info.yml and I'm not going to code it. Using modulename.info.yml fixes the false positive problem and it doesn't require any extra code.
Comment #82
pounard@#81 You are not supposed to edit 10 module info files at the same time (or rarely), plus we could say the same thing about composer.json files. I think the fixed info file name is OK, the only limitation it brings is that every module must have its own folder (which makes sense too) and if I remember correctly it is already a D8 constraint.
Comment #83
cweagansEven two or three would be annoying, and I frequently have a few .info files open at the same time. It's not really that far-fetched.
Which is one of the reasons that I'm not particularly enthused about composer.
As I said, I'm not writing the code to have a fixed filename. We've already strayed way outside what this issue was supposed to be for: a simple syntax conversion from our current info format to yaml. The .info.yml naming change was necessary so that we're not trying to parse CMI YAML as module metadata and that's a legitimate change, but having a bunch of module-info.yml files is outside the scope of this issue and outside my realm of interest, so if that's what has to be done to get rid of our info format, count me out.
In the meantime, I'll be rerolling the patch in #67 and fixing tests.
Comment #84
sunI actually really like how a static + always identical filename would enforce the new one-extension-per-directory rule for D8:
#1299424: Allow one module per directory and move system tests to core/modules/system
I didn't think of that consequence yet, and I must say that it makes the
extension.yml
a lot more appealing, because this is actually a hard requirement for D8 now, and we have no way to enforce it yet.This would be a super trivial way to enforce it, which is even self-documenting on top.
EDIT: To clarify what is meant here:
extension.yml
is meant literally; i.e., "extension" is NOT to be replaced with the extension's name.extension.yml
(or perhaps evencomposer.yml
?) file for each extension would inherently enforce the architectural requirement that is not enforced in any way yet.Comment #85
pounardAgree with you about this one.
Comment #86
cweagansComment #87
cweagansI'm not going to continue arguing about a filename. My goal was to get rid of our .info parser. That's clearly not going to happen because we're going to scope creep this to death. Somebody else can deal with this.
Comment #88
jwilson3What if we first get the basic requirement of YAML format working, per this issue, then create a follow-up issue that someone else may be interested in for converting from modulename.info.yml to module-info.yml? These seem like two fairly separate and clean-cut issues with different goals, thus easy to separate, and commit in separate phases. The only unfortunate thing about doing it this way would be two change notices switching things up twice for other devs, but it wouldnt be the end of the world, core Drupal 8 is in a LOT of flux right now, if the follow-up got in quickly, i doubt anyone would even notice ;)
Comment #89
tstoecklerI also agree with #84, but until themes are actually full extensions (#1067408: Themes do not have an installation status), and we actually have the concept of extensions in core (#1331486: Move module_invoke_*() and friends to an Extensions class), I think it would make sense to go with
module.yaml
for now, instead ofextension.yaml
. Since the latter would also require parsing each file to know it is a module (and introducing atype
key for that purpose), I would consider that feature-creep, even it is the intended goal in the end.Comment #90
tstoecklerIn theory we could split the issues, but I actually don't consider that feature-creep. We have already established that we need to rename the files anyway, so figuring out what to rename them to, needs to be part of that discussion.
Comment #91
joachim CreditAttribution: joachim commented> The only unfortunate thing about doing it this way would be two change notices switching things up twice for other devs, but it wouldnt be the end of the world,
Change notices can be amended, and merged, as one change notice can cover several issues. I don't see a problem with that.
Comment #92
cweagansThis issue is not, and has never been, about enforcing the one module per directory rule. It's not about using a static name for all module info files. It's about getting rid of our janky-ass regex parser function and replacing it with something semi-maintainable.
I will never support moving to a static filename for metadata, and I will likely never support using Composer for the same reason (in addition to the fact that Composer will likely require using JSON instead of YAML). Having a bunch of duplicate filenames is a pain in the ass (I worked on a project for a past employer that had a similar system, and each module (all 300 of them) in the codebase had a metadata.ini file - I'm pretty sure I spent more time going through editor tabs trying to find the right one than I did actually writing code). So if that's what's going to happen in this issue (or others), I don't want any part of it.
As I said, somebody else can deal with it.
Comment #93
Crell CreditAttribution: Crell commentedI'm very enthused about Composer, but as I said above that's a follow-up. I'm fine with keeping this issue focused as cweagans suggests on just removing the custom parsing code, vis, reducing the number of lines in Drupal.
Focus, people! :-)
Comment #94
patcon CreditAttribution: patcon commentedAck! I'm really sorry I derailed this! You're totally right that this is a separate issue @cweagans, if only just that it's commanding opinions in two very distinct domains. Better to keep them separate to we can focus energies productively
POOF. A wild core issue appears!
#1810572: Move info-file from variable format (modulename.yml) to static format (module.yml / extension.yml)
PS, I love you cweagans. From the bottom of my open-sourcey heart. And yes, this is how I talk to people in real life too :)
Also, hey everyone! here's a picture of a majestic bear: http://bit.ly/Pivtth
patcon shrugs.
Comment #95
cweagansRemoving tags for now
Comment #96
sunDue to the massive file changes, I moved this patch into the framework-info-1793074-sun branch in the Platform sandbox.
I also reverted all .info file changes in that branch, and added a conversion script instead, which allows a total patch creation process like this:
Reverted all .info files.
Added conversion script.
Replaced .yml with .info.yml.
Removed obsolete InfoFileParserUnitTest.
Replaced .yml with .info.yml in aaa_update_test.tar.gz.
Reviewing the patch myself, I just realized that the scripted conversion removes all comments previously contained in .info files. We either have to re-inject them manually - or - most probably it would be a much better idea to get rid of them entirely and move them somewhere more appropriate (PHP).
Comment #98
cweagansIn the conversion script, you need to change your Yaml::dump($info); call to Yaml::dump($info, 15, 2); This will force the YAML dumper to never use the inline syntax, and to use a 2 space indentation.
Not sure why installation is failing, but I may be able to take a look later.
Comment #99
sun@cweagans: FWIW, I just verified, and you have write access to http://drupal.org/sandbox/sun/1255586 already.
Comment #100
alexpottWe need to make the resulting YAML files obey the Drupal code formatting conventions.
core/scripts/convert-info-yml.php
could do this by doing:Comment #101
cweagansComment #102
cweagansI've tracked down the problem with the installation. The issue is that we're using pathinfo() to get the file name without the extension, but it's giving us everything up to the last extension (that is, foo.tar.gz 's filename is foo.tar), so this breaks a lot of things.
That said, I'm going to postpone this until after feature freeze. There's no reason that we need to spend time on this right now.
Comment #103
cweagansGoodbye.
Comment #104
cweagansPicking this back up.
Comment #105
chx CreditAttribution: chx commentedCameron, welcome back!
Comment #106
cweagansThanks chx. I couldn't stay away :)
Comment #107
cweagansNew patches. Let's see how badly these fail review. I rerolled this by hand, so my apologies if there are issues that have been fixed in previous patches.
The do-not-patch test does not include the .info -> .info.yml renames and conversions. It's just the Drupal-related changes that have to happen. The big patch is all of the Drupal changes + the .info -> .info.yml conversions.
Comment #109
cweagansAh, right. Okay, so the problem that's happening is the same problem that I pointed out in #102. Not sure how to deal with this. I'll think about it and reroll when I can.
Comment #110
cweagansKeeping up with head (conflict in install.core.inc)
Comment #111
Crell CreditAttribution: Crell commentedComment #113
cweagansYeah, I know these patches are going to fail. I'm not sure how to handle the pathinfo() stuff. Basically, what I'm getting is this: http://monosnap.com/image/KRqX1omK8EhqCJF8PJ0YsgUJ6
Comment #114
cweaganschx totally fixed the issue with the filenames. Please make sure he gets commit credit :)
Comment #115
cweaganschx also pointed out that we definitely need doxygen on SystemListing->processFile(), so this is definitely needs work still.
Comment #116
cweagansDocs.
Comment #118
star-szr@cweagans, thanks for all your work on this. If it's feasible I'd love to see an interdiff to show how you were able to get this installed!
Comment #119
cweagansInterdiff between 110 and 114 is roughly http://sprunge.us/LUed, I think.
Comment #120
sunThe interdiff showed that the previous code removed '.info.yml' via basename() here, which might explain the test failures.
Comment #121
cweagansActually, if I change that to .info.yml, I can't even install Drupal.
I did, however, notice a few new .info files that needed converting to YAML, so I did that. Hopefully, that reduces the number of failures.
Comment #123
cweagansOr not.
Comment #124
cweagansReroll to convert these to YAML:
./core/modules/ckeditor/ckeditor.info
./core/modules/ckeditor/tests/modules/ckeditor_test.info
./core/modules/menu_link/menu_link.info
./core/modules/serialization/serialization.info
./core/modules/serialization/tests/serialization_test/serialization_test.info
Probably going to fail again, but why not?
Comment #126
cweagansNew patch and interdiff attached.
Comment #128
cweagans#126: 1793074_126-convert-info-to-yml.patch queued for re-testing.
Comment #130
cweagans- Adding datetime to dependencies lists
- Removing bunnies from menu_link.info.yml
Comment #131
cweagansComment #132
amateescu CreditAttribution: amateescu commentedWhyyyy? I was so proud that I managed to sneak that in! And it's unrelated to this issue :D
Comment #133
cweagansIt was removed elsewhere! I totally would have left it in! http://drupal.org/node/1918920
Comment #134
cweagansI suppose that it would help to actually convert datetime.info...
Comment #136
cweagansThis should fix that test (the .info.yml in aaa_update_test.tar.gz was empty).
Comment #137
cweagansReroll for tour module changes.
Comment #139
cweagansAnd another for changes in options.info...
Comment #140
cweagansAside from the .info rewriting to .info.yml, these files changed to account for it:
Comment #140.0
cweagansAdding todos to issue summary
Comment #141
cweagansSorry for the noise. This is a pain to reroll, so tagging.
Comment #142
chx CreditAttribution: chx commentedThis does look good to me. The real change is minimal: constant support is dropped as per issue summary and an per file process callback is added to systemlist because basename(foo.info.yml) yields foo.info and we need foo.
Comment #143
star-szrThis looks good, would love to see this in.
These should end in a period.
Other than that, just some inline comments that should be wrapped at 80 chars.
Comment #144
cweagansFixed the comment mentioned above. Cottser is making a list of other changes to make, so waiting to upload a new patch for a minute.
Comment #145
webchickThis makes sense to me and eliminates 1 of the at least 5 distinct ways of registering metadata in Drupal 8. ;)
However, a change this big could really use sign-off from the man upstairs, so assigning. I'm also tagging to note that this was RTBC on Feb 18.
Comment #146
Dave ReidI can't remember if our YAML implementation is vulnerable to the serialization attack: http://osvdb.org/89621. This would be particularly troubling since .info.yml files are not required to have their modules enabled in order to be parsed. I can't remember if our version is vulnerable or not. If it is, this is kind of a huge security concern. Since at worst what you could do with .info files is XSS, but this would execute code.
Comment #147
cweagansOkay, new patch and two interdiffs attached. interdiff.txt is the 80 column wrapping fixes that Cottser pointed out. yaml_security_interdiff.txt is the security fix needed to address 146, and the new patch incorporates all of that.
Comment #148
cweagansPer 145
Comment #149
cweagansRelated: #1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available
(Can be taken care of after this issue, though)
Comment #150
podarok#147 green
+1 RTBC ( see no errors after manual review )
Comment #151
catchiirc the modules page parses .info files every request for every module on the system.
It'd be interesting to get a before/after xhprof comparison for this.
Not so much for this issue, but for #1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available - once this is in it'll be the first place that YAML files get scanned without subsequently being cached somewhere.
Comment #152
cweagansI....have no idea how to do that. I'll see if I can come up with something
Comment #153
cweagansmsonnabaum pointed me toward https://gist.github.com/msonnabaum/4711730, so I'll give that a go in a while.
Comment #154
cweagansI'm not sure why this was classified as a feature request (I guess I did that when I opened the issue). This really doesn't seem feature-y to me.
Comment #155
sunComment #156
cweagansRerolling to account for http://drupalcode.org/project/drupal.git/blobdiff/8d029e84b4bf9016652316...
Comment #157
cweagansRerolling to convert config_integration_test.info and condition_test.info to yaml
Comment #158
hass CreditAttribution: hass commentedA changed
aaa_update_test.tar.gz
looks like an invalid change.Comment #159
ParisLiakos CreditAttribution: ParisLiakos commented@hass: you mean the binary change? there is a compressed module with an info file that needs renaming
Comment #160
cweagansProfiling:
Before: http://monosnap.com/image/QOnqI4W95kCvLMxV6S5vhHM47
After: http://monosnap.com/image/yAxU87W3Zbs9wYaB2xqlFAX7k
Is that all the information needed? Or do you want the full call graph?
Comment #161
cweagansYeah, the aaa_update_test.tar.gz is intentional. That discussion has happened twice in IRC now. There was a .info file in there that needed changed.
Comment #162
hass CreditAttribution: hass commentedUps, sorry than. Why are we making Drupal slower and run 80k functions more? :-)
Comment #163
cweagansI don't think this is a big deal. These numbers came from the Modules page, which is really the only page that should be affected by this change.
Comment #164
cweagansxhprof files attached. I also attached the output of the xhprof diff, in case anyone wants to take a look without running xhprof.
Comment #165
neclimdulIt should come as no surprise that yaml parsing adds function calls and quite a bit of overhead. This was well documented in the file format thread of early CMI days http://groups.drupal.org/node/167584
I broke the benchmarks down a little and here are the exact differences. Memory is ignored since it goes down.
For a realistic break down lets look at per module information so we can see how the page load would grow with more modules.
That breaks down to:
~398 function calls / module.
~1615 ms / module
The important point here is that like cweagans said, this is only really important on the module page. We actually discussed this "parsing files is slow" problem when we added ini files too and the fact is it doesn't happen in any critical code paths so its not really an issue as long as it doesn't kill the module page. These changes are not insignificant but its also not likely to break the module page because even if they added another 200 modules to their site, we're still talking under a second added time over the current implementation. ~.6 seconds. Also memory is down, and that's possibly a bigger concern since we're parsing for enabled and disabled modules.
All in all, I don't see performance as a blocker on this.
Comment #166
sunI agree that performance should not be of any concern here.
The main focus is on DX and consistency. Drupal's custom .info file format is just simply an alien to everyone who has never worked with Drupal. People can immediately make sense of YAML, JSON, INI, XML, PHP, and everything else. The INI format is fundamentally not able to cope with the data we're trying to specify. Therefore, YAML comes closest as a replacement in terms of DX, TX, common sense, and general sanity. And it should be noted that YAML has proven itself as an excellent serialization format that is able to cope with all challenges we're facing. Furthermore, we have a range of new $module.routing.yml as well as configuration files in the YAML format in Drupal 8.
But what ultimately matters is that we're switching to an industry standard format.
In light of that: Formerly raised concerns about "each and every Drupaler having to re-learn how to write '.info' files" are merely a sole artifact of bad habits. I've to admit that I do not know the historical details, but I'm fairly confident that our .info file format was caused by the limitations of the natively supported INI file format. Thus, Drupal went ahead and forged the INI file format in a custom one, which supports what we're trying to express, but which still follows the INI syntax rules for the most part. This act must date back to earlier than 2005, since I'm not aware of a Drupal that lived without it. Anyway — The moment you can forget about Drupal's utterly custom .info file parsing algorithms, the world turns into a better place.
And who knows... together with #1292284: Require 'type' key in .info.yml files, Drupal might very well be the first PHP app that introduces YAML support for Composer at some point? As discussed and documented in #1398772: Replace .info.yml with composer.json for extensions, Composer natively supports alternative formats — it's merely bound to JSON right now, because no one looked into YAML support thus far. Long story short, this switch can be a major milestone for Drupal but also for the PHP community at glance.
Any possible performance impacts can be resolved on multiple levels — starting from improving and optimizing the upstream YAML parser library, on to #1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available, and ultimately, there's nothing that would prevent us from actually caching the .info.yml files even for the Modules page (and adding a button there to refresh the cache). In any case, KISS trumps Drupalisms.
Thanks, @cweagans for sticking with this! I almost lost hope when you marked it won't fix some time ago, and I'm glad that we're even in feature-freeze deadline. Well done! :)
That said, I don't understand 1) why this task is related to feature freeze in any way, and 2) why this task is not major.
Comment #167
sunAlso, in terms of architectural API clean-up:
I want to stress some more on the detail that is buried in #84. I just edited that comment to clarify the problem space and why this move from
whatever.info
towhatever.info.yml
might only be of a temporary nature.However, that additional clean-up will only be a pure rename of the existing files and should be done in a follow-up. It also does not change the importance of switching from our custom .info file format to an industry standard format.
Comment #168
cweagansIMO, it's not at all related to feature freeze. But it was done before feature freeze, so even if it is, it's still eligible :)
I'm totally okay with using composer if we're also using YAML, and that seems to be the main hangup in the other issue. I hope Dries can take a look at this soon. I really don't want to have to keep rerolling this. :)
Comment #169
Crell CreditAttribution: Crell commentedHistorical side note in relation to #166: info files were the brain child of mad scientist Steven Wittens, and were introduced in, I believe, Drupal 5. "ini files on steroids" is a fairly accurate description of their genesis. The other major contender at the time was XML; the two main objections to that were "XML is too hard for people" (which was and remains a specious argument) and "PHP 4's XML parsing is so bad that parsing a custom ini++ format is actually easier" (which was a legit point at the time).
YAML-in-Composer is off topic for this issue. Otherwise, +1 to the rest of #166. Let's get this in before it breaks again. :-)
Comment #170
chx CreditAttribution: chx commentedDrupal's info format is a very direct descendant of parse_ini_file (I daresay it's practically the same) so it should not be that alien. The problem is
parse_ini_file()
itself is so bizarre and inconsistent that it never got traction in the PHP world -- Steven made it consistent and friendly but alas noone else jumped on this bandwagon so that's where we are.I am very happy with going to YAML.
Comment #171
patcon CreditAttribution: patcon commentedsun++ for #166
Not for followup discussion here, but reference -- For anyone interested in yaml and composer:
https://github.com/composer/composer/issues/440
Comment #172
neclimdultechnically, info files where my, merlin and webchicks brain child #80952: Add .info files to modules to store meta information. As chx said, Steven wrote the parse_info implementation. We seem to be in agreement though :)
Comment #173
cweagans#157: 1793074_157-convert-info-to-yaml.patch queued for re-testing.
Comment #174
cweagansI requeued the test because webchick says that she and Dries have a D8 meeting tomorrow and this is on the docket. Just making sure that everything still applies cleanly and tests pass and all that.
Comment #176
chx CreditAttribution: chx commentedRerolled. We can't miss our window, can we?
Comment #177
Dries CreditAttribution: Dries commentedThis works for me. Committed to 8.x. Thanks.
Comment #178
cweagansChange notice: http://drupal.org/node/1935708
Comment #179
cweagansRemoving tags.
Comment #180
Gábor HojtsyThis will have some ripple effects in tasks in the packaging system on d.o (which modifies .info files with date and project info when packaging) as well as localize.drupal.org's backend where these files are parsed and drush. I've opeend #1936028: Support for Drupal 8 info.yml translatables for l.d.o's backend (among numerous other changes needed to make Drupal 8 translatable).
Comment #181
sunAny .module file in the filesystem that does not have a corresponding .info.yml throws an exception now:
Comment #182
sunAlso created follow-up issue for #84: #1936886: Rename $module.info.yml into extension.yml
Comment #183
alexpottWe already check for the file's existence in drupal_parse_info_file... perhaps this is a simpler and more performant fix (less accessing the filesystem)...
Comment #184
chx CreditAttribution: chx commentedComment #185
catchCommitted/pushed the followup. Thanks!
Comment #187
dwwI know I'm late to the party, but I'm so happy to see this in core. .info files were originally ini compatible. They got mad scientist-ified at #132018: Add .info files to themes (and improve/clean up the theme system). A few of us actually tried to switch to YAML at that point, but we were unsuccessful, in part due to this gem from Crell circa April 2007 (comment #47):
;) See, sometimes core development really is fun! :)
Anyway, 6 years later, I'm happy to see this done!
Although a few people mentioned it, I didn't see that anyone actually opened an issue about fixing the packaging system to deal with this, so I just did:
#1963092: Convert .info file rewriting in packaging plugins to deal with D8 .info.yml files
"The d.o packaging script" is now pluggable, and all the d.o-specific stuff lives in plugins provided by the drupalorg project.
Anyway, thanks to everyone who worked on this, especially cweagans!
Cheers,
-Derek
Comment #188
xjmWhile the change notice for this is great, we also need to update a bunch other change notices that have
.info
examples in them:http://drupal.org/node/1397856
http://drupal.org/node/1626346
http://drupal.org/node/1632592
http://drupal.org/node/1830116
http://drupal.org/node/1876152
http://drupal.org/node/1876600
http://drupal.org/node/1911646
All of the above need to be updated to use the new
.info
file syntax for their D8 examples. You can also add this issue to the list of issues each of the above references.Comment #189
cweagansOn it.
Comment #190
cweagansI updated all of the ones that xjm mentioned except for [#1626346]. That looks like a D7 entry. Does that need to be updated?
Comment #191
xjm@cweagans Right you are. Thanks for the quick turnaround!
Comment #192
Crell CreditAttribution: Crell commentedROFL! In my defense, dww, I still don't actually like YAML and in 2007 I was looking at Symfony 1. ;-)
Comment #193
ParisLiakos CreditAttribution: ParisLiakos commentedrestoring title
Comment #194
dwwFixing the title for posterity and laughing back with Crell. :)
Thanks again, everyone...
-Derek
Comment #196.0
cweagansUpdating todo list and adding suggested commit message.
Comment #197
xjm