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

CommentFileSizeAuthor
#183 1793074.fix-info.183.patch1.07 KBalexpott
#181 _fix-info.patch1.28 KBsun
#176 1793074_157-convert-info-to-yaml.patch248.1 KBchx
#164 before.txt617.33 KBcweagans
#164 after.txt635.87 KBcweagans
#164 diff_view.html1.44 MBcweagans
#157 1793074_157-convert-info-to-yaml.patch248.07 KBcweagans
#156 1793074_156-convert-info-to-yaml.patch245.81 KBcweagans
#147 1793074_147-convert-info-to-yaml.patch245.82 KBcweagans
#147 interdiff.txt13.57 KBcweagans
#147 yaml_security_interdiff.txt1.02 KBcweagans
#139 1793074_139-convert-info-to-yaml.patch243.1 KBcweagans
#137 1793074_137-convert-info-to-yaml.patch243.46 KBcweagans
#136 1793074_136-convert-info-to-yaml.patch242.88 KBcweagans
#134 1793074_134-convert-info-to-yml.patch242.04 KBcweagans
#130 1793074_130-convert-info-to-yml.patch241.32 KBcweagans
#126 1793074_126-convert-info-to-yml.patch239.39 KBcweagans
#126 interdiff.txt2.4 KBcweagans
#124 1793074_124-convert-info-to-yml.patch238.72 KBcweagans
#121 1793074_121-convert-info-to-yml.patch234.61 KBcweagans
#116 1793074_116-convert-info-to-yml.patch229.54 KBcweagans
#116 interdiff.txt1.02 KBcweagans
#114 1793074_114-convert-info-to-yml.patch229.42 KBcweagans
#110 1793074_110-convert-info-to-yml.patch227.89 KBcweagans
#107 1793074_107-convert-info-to-yml.do-not-test.patch71.37 KBcweagans
#107 1793074_107-convert-info-to-yml.patch227.58 KBcweagans
#96 framework.info.96.do-not-test.patch21.63 KBsun
#96 interdiff.txt17.85 KBsun
#96 framework.info.96.patch143.98 KBsun
#67 1793074_67-convert_info_to_yml.patch138.75 KBcweagans
#63 1793074_convert_info_to_yml.patch127.62 KBcweagans
#25 1793074_25-info_to_yaml.patch97.55 KBcweagans
#23 1793074_23-info_to_yaml.patch97.32 KBcweagans
#21 1793074_21-info_to_yaml.patch97.05 KBcweagans
#19 1793074_19-info_to_yaml.patch96.5 KBcweagans
#17 1793074_17-info_to_yaml.patch96.73 KBcweagans
#14 1793074_14-info_to_yaml-excludes-info-files-do-not-test.patch9.15 KBcweagans
#14 convert-info-yml.php_.txt13.06 KBcweagans
#14 1793074_14-info_to_yaml.patch95.82 KBcweagans
#10 1793074_10_common.inc-do-not-test.patch5.42 KBcweagans
#10 1793074_10-info_to_yaml.patch92.31 KBcweagans
#10 convert-info-yml.php_.txt13.05 KBcweagans
#6 1793074_6-info_to_yaml.patch92.42 KBcweagans
#1 1793074_common.inc_.patch5.33 KBcweagans
#1 1793074_1-info_to_yaml.patch91.78 KBcweagans
#1 convert-info-yml.php_.txt13.01 KBcweagans
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cweagans’s picture

Status: Active » Needs review
FileSize
13.01 KB
91.78 KB
5.33 KB

Okay, 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.

Status: Needs review » Needs work

The last submitted patch, 1793074_1-info_to_yaml.patch, failed testing.

joachim’s picture

+++ b/core/modules/forum/forum.info
@@ -1,10 +1,12 @@
-dependencies[] = node
-dependencies[] = taxonomy
-dependencies[] = comment
 ...
+dependencies:
+    - node
+    - taxonomy
+    - comment

My 2p is that this makes arrays in .info files easier to read and write.

cweagans’s picture

You think that's good, check out bartik.info (especially regions and stylesheets) :)

cweagans’s picture

Testbot error is http://paste2.org/p/2260152. I'm not sure what the problem is. :(

cweagans’s picture

Status: Needs work » Needs review
FileSize
92.42 KB

Trying something else.

Status: Needs review » Needs work

The last submitted patch, 1793074_6-info_to_yaml.patch, failed testing.

kika’s picture

module.info or module.info.yml ?

cweagans’s picture

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

cweagans’s picture

New patch to account for ban.module. Also added substitution for the VERSION constant in common.inc.

cweagans’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1793074_10-info_to_yaml.patch, failed testing.

joachim’s picture

+++ b/core/includes/common.inc
@@ -6561,105 +6563,13 @@ function drupal_parse_info_file($filename) {
+      if (isset($info[$filename]['version']) && $info[$filename]['version'] == 'VERSION') {
+        $info[$filename]['version'] = VERSION;

This magic should be mentioned in the function docs.

cweagans’s picture

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

cweagans’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1793074_14-info_to_yaml.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review
FileSize
96.73 KB

Forgot the common_test_info.txt changes.

Status: Needs review » Needs work

The last submitted patch, 1793074_17-info_to_yaml.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review
FileSize
96.5 KB

One more. This should fix InfoFileParserUnitTest

Status: Needs review » Needs work

The last submitted patch, 1793074_19-info_to_yaml.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review
FileSize
97.05 KB

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

Status: Needs review » Needs work

The last submitted patch, 1793074_21-info_to_yaml.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review
FileSize
97.32 KB

Fixed multiline YAML syntax

Status: Needs review » Needs work

The last submitted patch, 1793074_23-info_to_yaml.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review
FileSize
97.55 KB

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

cweagans’s picture

.

kika’s picture

AFAIR support for multiline strings were one of the reasons why Steven implemented drupal_parse_info_format()

kika’s picture

fixing tag

cweagans’s picture

Re #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.

RobW’s picture

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

cweagans’s picture

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

Lars Toomre’s picture

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

chx’s picture

Note: the current info parser uses get_defined_constants which is a memory hog if I ever saw one.

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/includes/common.inc
@@ -6537,11 +6540,11 @@ function drupal_array_nested_key_exists(array $array, array $parents) {
- * - base: Name of a base theme, if applicable; e.g., base = zen.
- * - regions: Listed regions; e.g., region[left] = Left sidebar.
- * - features: Features available; e.g., features[] = logo.
- * - stylesheets: Theme stylesheets; e.g., stylesheets[all][] = my-style.css.
- * - scripts: Theme scripts; e.g., scripts[] = my-script.js.
+ * - base: Name of a base theme, if applicable
+ * - regions: Listed regions
+ * - features: Features available
+ * - stylesheets: Theme stylesheets
+ * - scripts: Theme scripts

These changes seem unrelated?!

+++ b/core/modules/system/tests/themes/test_theme/test_theme.info
@@ -1,18 +1,8 @@
-; Normally, themes may list CSS files like this, and if they exist in the theme
-; folder, then they get added to the page. If they have the same file name as a
-; module CSS file, then the theme's version overrides the module's version, so
-; that the module's version is not added to the page. Additionally, a theme may
-; have an entry like this one, without having the corresponding CSS file in the
-; theme's folder, and in this case, it just stops the module's version from
-; being loaded, and does not replace it with an alternate version. We have this
-; here in order for a test to ensure that this correctly prevents the module
-; version from being loaded, and that errors aren't caused by the lack of this
-; file within the theme folder.

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.

cweagans’s picture

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

sun’s picture

Doesn'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.

cweagans’s picture

Sure, 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.

tstoeckler’s picture

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

tenken’s picture

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

cweagans’s picture

@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)

joachim’s picture

Issue tags: +Needs change record

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

sun’s picture

Issue tags: -Needs change record
+++ b/core/modules/aggregator/aggregator.info
@@ -1,8 +1,10 @@
+  all: [aggregator.theme.css]

+++ b/core/modules/book/book.info
@@ -1,8 +1,10 @@
+  all: [book.theme.css]

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

+++ b/core/modules/ban/ban.info

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 rather composer.json...

(I know... I'm terribly sorry :-/)

sun’s picture

Crell’s picture

I'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.

cweagans’s picture

I would like to mirror this sentiment: http://drupal.org/node/1398772#comment-5527088

So, composer.json as a clean packaging metadata, yes. composer.json as a complete replacement for our info files, I don't think so.

That's where YAML comes in, IMO.

Crell’s picture

Moving 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...)

cweagans’s picture

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

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

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.

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

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.

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

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.

sun’s picture

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

cweagans’s picture

Putting YAML into an .info file is like putting JSON into an .xml file

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

joachim’s picture

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

patcon’s picture

@joachim

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.

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 a packages.json, etc :) < /bikeshed>

EDIT: I'm with Crell on the larger issue.

Crell’s picture

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

Lars Toomre’s picture

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

cweagans’s picture

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

I already have to configure my IDE to understand .info. I tell it plain text. Now I'd have to tell it to use YAML.

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.

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:

...
regions:
  - page_top: Page top
  - page_bottom: Page bottom
 ...

rather than

...
  "regions": {
    "page_top": "Page top",
    "page_bottom":  "Page bottom"
  },
...

@#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.

chx’s picture

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

pounard’s picture

IDEs can add file extensions. Few IDE recognizes .module as PHP and yet we live. That's a non-issue.

I'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.

joachim’s picture

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

a.ross’s picture

IMO, 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

bhauff’s picture

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

Crell’s picture

To 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".

RobLoach’s picture

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

Fully agree utilizing Composer for the package information would be huge, but what's blocked on the Composer front?

cweagans’s picture

what's blocked on the Composer front?

Well, the issue has been open since January and there have been no patches posted, so there's one thing.

cweagans’s picture

Status: Needs work » Needs review
FileSize
127.62 KB

Let'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.

Status: Needs review » Needs work

The last submitted patch, 1793074_convert_info_to_yml.patch, failed testing.

star-szr’s picture

Issue tags: +Needs change record

Re-adding change notification tag.

jwilson3’s picture

Added 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).

cweagans’s picture

Status: Needs work » Needs review
Issue tags: +Avoid commit conflicts
FileSize
138.75 KB

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

Status: Needs review » Needs work

The last submitted patch, 1793074_67-convert_info_to_yml.patch, failed testing.

kika’s picture

Just curious, what's the reasoning behind leading "---" ?

cweagans’s picture

http://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".

joachim’s picture

Status: Needs work » Needs review

I really don't want to bikeshed, but *.info.yaml would keep the bit that tells us what the file does.

cweagans’s picture

Status: Needs review » Needs work

IMO, 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

joachim’s picture

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

pounard’s picture

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

contrib/modules/mymodule_views.module
contrib/modules/mymodule_searchapi.module
third-party/modules/mymodule_mongo.module
mymodule.module
mymodule_ui.module

Accompagnied with an index package.yml file such as:

package:
    - name: My module
    - description: Provide my über cool feature
    - version: 8.1.0-rc1
modules:
    - mymodule:
        - name: My module
        - path: .
        - dependencies:
            - menu: =8.*
    - mymodule_views:
        - name: My module integration with Views
        - path: contirb/modules
        - dependencies:
            - mymodule: *
            - views: =8.*
    - mymodule_searchapi:
        - name: My module integration with Search API
        - path: contirb/modules
        - dependencies:
            - mymodule: *
            - searchapi: =8.2.*
    - mymodule_mongo:
        - name: My module third party integration to Mongo
        - path: third-party/modules
        - dependencies:
            - mymodule: *
            - mongo: >=8.3.1

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.

sun’s picture

1) 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.

Crell’s picture

pounard: 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. :-)

RobLoach’s picture

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

cweagans’s picture

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

sun’s picture

That sounds like an acceptable stop-gap/workaround fix, yes. However, I wouldn't want to see that in the final D8 release.

patcon’s picture

What about just choosing a static filename like module-info.yml and having it consistent everywhere? Why is the focus on using modulename[.info].yml and then having a way to quickly weed out imposters? Isn't the first approach simpler?

cweagans’s picture

@#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.

pounard’s picture

@#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.

cweagans’s picture

You are not supposed to edit 10 module info files at the same time (or rarely)

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

we could say the same thing about composer.json files

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.

sun’s picture

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

  1. extension.yml is meant literally; i.e., "extension" is NOT to be replaced with the extension's name.
  2. This inherently prevents two extensions to live in the same directory.
  3. That's a critical aspect of D8's new architecture, but it is not enforced anywhere.
  4. Therefore, an extension.yml (or perhaps even composer.yml?) file for each extension would inherently enforce the architectural requirement that is not enforced in any way yet.
  5. intuitive++
pounard’s picture

Agree with you about this one.

cweagans’s picture

Assigned: cweagans » Unassigned
cweagans’s picture

I'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.

jwilson3’s picture

What 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 ;)

tstoeckler’s picture

I 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 of extension.yaml. Since the latter would also require parsing each file to know it is a module (and introducing a type key for that purpose), I would consider that feature-creep, even it is the intended goal in the end.

tstoeckler’s picture

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

joachim’s picture

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

cweagans’s picture

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

Crell’s picture

I'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! :-)

patcon’s picture

Ack! 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.

cweagans’s picture

Issue tags: -Avoid commit conflicts

Removing tags for now

sun’s picture

Status: Needs work » Needs review
FileSize
143.98 KB
17.85 KB
21.63 KB

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

# Create patch + interdiff for review.
git diffup framework.info.96.do-not-test.patch

# Convert all .info files.
find -path '*core*' -name '*.info' | xargs php core\scripts\convert-info-yml.php

# Create complete patch and revert everything.
git diff 8.x > framework.info.96.patch
git reset --hard

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

Status: Needs review » Needs work

The last submitted patch, framework.info.96.patch, failed testing.

cweagans’s picture

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

sun’s picture

@cweagans: FWIW, I just verified, and you have write access to http://drupal.org/sandbox/sun/1255586 already.

alexpott’s picture

We need to make the resulting YAML files obey the Drupal code formatting conventions. core/scripts/convert-info-yml.php could do this by doing:

use Symfony\Component\Yaml\Dumper;

// Customize if needed.
$git_path = 'git';

define('DRUPAL_ROOT', getcwd());

require_once DRUPAL_ROOT . '/core/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION);

$info_files = $_SERVER['argv'];
array_shift($info_files);

$dumper = new Dumper();
// Set Yaml\Dumper's default indentation for nested nodes/collections to
// 2 spaces for consistency with Drupal coding standards.
$dumper->setIndentation(2);

foreach ($info_files as $info_file) {
  $info = file_get_contents($info_file);
  $info = drupal_parse_info_format($info);
  // The level where you switch to inline YAML is set to PHP_INT_MAX to ensure
  // this does not occur.
  $yaml = $dumper->dump($info, PHP_INT_MAX);
  $yaml_file = $info_file . '.yml';
  file_put_contents($yaml_file, $yaml);
  system($git_path . ' rm ' . escapeshellarg($info_file));
  system($git_path . ' add ' . escapeshellarg($yaml_file));
}
cweagans’s picture

Assigned: Unassigned » cweagans
cweagans’s picture

Status: Needs work » Postponed

I'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.

cweagans’s picture

Assigned: cweagans » Unassigned

Goodbye.

cweagans’s picture

Assigned: Unassigned » cweagans
Status: Postponed » Active

Picking this back up.

chx’s picture

Cameron, welcome back!

cweagans’s picture

Thanks chx. I couldn't stay away :)

cweagans’s picture

Status: Active » Needs review
FileSize
227.58 KB
71.37 KB

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

Status: Needs review » Needs work

The last submitted patch, 1793074_107-convert-info-to-yml.patch, failed testing.

cweagans’s picture

Ah, 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.

cweagans’s picture

Keeping up with head (conflict in install.core.inc)

Crell’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1793074_110-convert-info-to-yml.patch, failed testing.

cweagans’s picture

Yeah, 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

cweagans’s picture

Status: Needs work » Needs review
FileSize
229.42 KB

chx totally fixed the issue with the filenames. Please make sure he gets commit credit :)

cweagans’s picture

Status: Needs review » Needs work

chx also pointed out that we definitely need doxygen on SystemListing->processFile(), so this is definitely needs work still.

cweagans’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
229.54 KB

Docs.

Status: Needs review » Needs work

The last submitted patch, 1793074_116-convert-info-to-yml.patch, failed testing.

star-szr’s picture

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

cweagans’s picture

Interdiff between 110 and 114 is roughly http://sprunge.us/LUed, I think.

sun’s picture

+++ b/core/lib/Drupal/Core/SystemListingInfo.php
@@ -69,4 +69,11 @@ protected function process(array $files, array $files_to_add) {
+  protected function processFile($file) {
+    $file->name = basename($file->name, '.info');
+  }

The interdiff showed that the previous code removed '.info.yml' via basename() here, which might explain the test failures.

cweagans’s picture

Status: Needs work » Needs review
FileSize
234.61 KB

Actually, 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.

Status: Needs review » Needs work

The last submitted patch, 1793074_121-convert-info-to-yml.patch, failed testing.

cweagans’s picture

Or not.

cweagans’s picture

Status: Needs work » Needs review
FileSize
238.72 KB

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

Status: Needs review » Needs work

The last submitted patch, 1793074_124-convert-info-to-yml.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review
FileSize
2.4 KB
239.39 KB

New patch and interdiff attached.

Status: Needs review » Needs work
Issue tags: -Proudly Found Elsewhere, -Needs change record

The last submitted patch, 1793074_126-convert-info-to-yml.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Proudly Found Elsewhere, +Needs change record

The last submitted patch, 1793074_126-convert-info-to-yml.patch, failed testing.

cweagans’s picture

- Adding datetime to dependencies lists
- Removing bunnies from menu_link.info.yml

cweagans’s picture

Status: Needs work » Needs review
amateescu’s picture

- Removing bunnies from menu_link.info.yml

Whyyyy? I was so proud that I managed to sneak that in! And it's unrelated to this issue :D

cweagans’s picture

It was removed elsewhere! I totally would have left it in! http://drupal.org/node/1918920

cweagans’s picture

I suppose that it would help to actually convert datetime.info...

Status: Needs review » Needs work

The last submitted patch, 1793074_134-convert-info-to-yml.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review
FileSize
242.88 KB

This should fix that test (the .info.yml in aaa_update_test.tar.gz was empty).

cweagans’s picture

Reroll for tour module changes.

Status: Needs review » Needs work

The last submitted patch, 1793074_137-convert-info-to-yaml.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review
FileSize
243.1 KB

And another for changes in options.info...

cweagans’s picture

Aside from the .info rewriting to .info.yml, these files changed to account for it:

#  modified:   core/includes/bootstrap.inc
#  modified:   core/includes/common.inc
#  modified:   core/includes/image.inc
#  modified:   core/includes/install.core.inc
#  modified:   core/includes/install.inc
#  modified:   core/includes/module.inc
#  modified:   core/includes/theme.inc
#  modified:   core/includes/theme.maintenance.inc
#  modified:   core/lib/Drupal/Core/SystemListing.php
#  modified:   core/lib/Drupal/Core/SystemListingInfo.php
#  modified:   core/lib/Drupal/Core/Updater/Updater.php
#  modified:   core/lib/Drupal/Core/Utility/ModuleInfo.php
#  modified:   core/modules/block/lib/Drupal/block/BlockListController.php
#  modified:   core/modules/block/tests/block_test.module
#  modified:   core/modules/breakpoint/tests/breakpoint_theme_test.module
#  modified:   core/modules/layout/tests/layout_test.module
#  modified:   core/modules/locale/locale.api.php
#  modified:   core/modules/locale/locale.compare.inc
#  modified:   core/modules/locale/locale.translation.inc
#  modified:   core/modules/locale/tests/modules/locale_test_translate/locale_test_translate.module
#  modified:   core/modules/system/lib/Drupal/system/Tests/Bootstrap/GetFilenameUnitTest.php
#  modified:   core/modules/system/lib/Drupal/system/Tests/Common/ParseInfoFileUnitTest.php
#  modified:   core/modules/system/lib/Drupal/system/Tests/FileTransfer/FileTransferTest.php
#  modified:   core/modules/system/lib/Drupal/system/Tests/Module/ModuleApiTest.php
#  modified:   core/modules/system/lib/Drupal/system/Tests/System/InfoAlterTest.php
#  modified:   core/modules/system/lib/Drupal/system/Tests/Theme/ThemeInfoStylesTest.php
#  modified:   core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
#  modified:   core/modules/system/system.admin.inc
#  modified:   core/modules/system/system.api.php
#  modified:   core/modules/system/system.module
#  modified:   core/modules/system/templates/region.tpl.php
#  modified:   core/modules/system/tests/common_test_info.txt
#  modified:   core/modules/system/tests/modules/ajax_test/ajax_test.module
#  modified:   core/modules/system/tests/modules/theme_page_test/theme_page_test.module
#  modified:   core/modules/system/tests/modules/theme_test/theme_test.module
#  modified:   core/modules/update/tests/aaa_update_test.tar.gz
#  modified:   core/modules/update/tests/modules/update_test/update_test.module
#  modified:   core/modules/update/update.api.php
#  modified:   core/modules/update/update.compare.inc
#  modified:   core/modules/update/update.fetch.inc
#  modified:   core/modules/update/update.module
cweagans’s picture

Issue summary: View changes

Adding todos to issue summary

cweagans’s picture

Issue tags: +Avoid commit conflicts

Sorry for the noise. This is a pain to reroll, so tagging.

chx’s picture

Status: Needs review » Reviewed & tested by the community

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

star-szr’s picture

This looks good, would love to see this in.

+++ b/core/includes/common.incundefined
@@ -6172,39 +6173,39 @@ function element_set_attributes(array &$element, array $map) {
- * - base: Name of a base theme, if applicable; e.g., base = zen.
- * - regions: Listed regions; e.g., region[left] = Left sidebar.
- * - features: Features available; e.g., features[] = logo.
- * - stylesheets: Theme stylesheets; e.g., stylesheets[all][] = my-style.css.
- * - scripts: Theme scripts; e.g., scripts[] = my-script.js.
+ * - base theme: Name of a base theme, if applicable
+ * - regions: Listed regions
+ * - features: Features available
+ * - stylesheets: Theme stylesheets
+ * - scripts: Theme scripts

These should end in a period.

Other than that, just some inline comments that should be wrapped at 80 chars.

cweagans’s picture

Fixed the comment mentioned above. Cottser is making a list of other changes to make, so waiting to upload a new patch for a minute.

webchick’s picture

Assigned: cweagans » Dries

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

Dave Reid’s picture

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

cweagans’s picture

Okay, 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.

cweagans’s picture

Issue tags: +RTBC Feb 18

Per 145

cweagans’s picture

podarok’s picture

#147 green
+1 RTBC ( see no errors after manual review )

catch’s picture

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

cweagans’s picture

I....have no idea how to do that. I'll see if I can come up with something

cweagans’s picture

msonnabaum pointed me toward https://gist.github.com/msonnabaum/4711730, so I'll give that a go in a while.

cweagans’s picture

Category: feature » task

I'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.

sun’s picture

Issue tags: +Extension system
cweagans’s picture

cweagans’s picture

Rerolling to convert config_integration_test.info and condition_test.info to yaml

hass’s picture

Status: Reviewed & tested by the community » Needs work

A changed aaa_update_test.tar.gz looks like an invalid change.

ParisLiakos’s picture

@hass: you mean the binary change? there is a compressed module with an info file that needs renaming

cweagans’s picture

Status: Needs work » Reviewed & tested by the community

Profiling:

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?

cweagans’s picture

Yeah, 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.

hass’s picture

Ups, sorry than. Why are we making Drupal slower and run 80k functions more? :-)

cweagans’s picture

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

cweagans’s picture

FileSize
1.44 MB
635.87 KB
617.33 KB

xhprof files attached. I also attached the output of the xhprof diff, in case anyone wants to take a look without running xhprof.

neclimdul’s picture

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

306,933 ms
75,615 function calls

For a realistic break down lets look at per module information so we can see how the page load would grow with more modules.

$ find . -name \*.info.yml  | wc -l
190

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.

sun’s picture

Priority: Normal » Major

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

sun’s picture

Also, 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 to whatever.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.

cweagans’s picture

IMO, 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. :)

Crell’s picture

Historical 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. :-)

chx’s picture

Drupal'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.

patcon’s picture

sun++ for #166

Not for followup discussion here, but reference -- For anyone interested in yaml and composer:
https://github.com/composer/composer/issues/440

neclimdul’s picture

technically, 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 :)

cweagans’s picture

cweagans’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1793074_157-convert-info-to-yaml.patch, failed testing.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
248.1 KB

Rerolled. We can't miss our window, can we?

Dries’s picture

Status: Reviewed & tested by the community » Fixed

This works for me. Committed to 8.x. Thanks.

cweagans’s picture

cweagans’s picture

Removing tags.

Gábor Hojtsy’s picture

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

sun’s picture

Status: Fixed » Needs review
FileSize
1.28 KB

Any .module file in the filesystem that does not have a corresponding .info.yml throws an exception now:

filemtime(): stat failed for sites/all/modules/edge/edge.info.yml

filemtime('sites/all/modules/edge/edge.info.yml')
_system_rebuild_module_data()
system_rebuild_module_data()
install_profile_modules(Array)
install_run_task(Array, Array)
install_run_tasks(Array)
install_drupal(Array)
Drupal\simpletest\WebTestBase->setUp()
Drupal\user\Tests\UserAdminRoleTest->setUp()
Drupal\simpletest\TestBase->run()
_simpletest_batch_operation(Array, '837', Array)
call_user_func_array('_simpletest_batch_operation', Array)
_batch_process()
_batch_do()
_batch_page()
system_batch_page()
call_user_func_array('system_batch_page', Array)
Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\HttpKernel->handle(Object, 1, 1)
Symfony\Component\HttpKernel\Kernel->handle(Object)
drupal_handle_request()
sun’s picture

alexpott’s picture

Category: task » bug
FileSize
1.07 KB

We 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)...

chx’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed the followup. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

dww’s picture

I 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):

As for YAML, Big -1. It's a huge and nasty mess. The only system I've toyed with that uses it is Symfony, and it translates it down to XML anyway. YAML is a pointless, less expressive mutant.

;) 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

xjm’s picture

Title: Convert .info files to YAML » Change notice updates: Convert .info files to YAML
Assigned: Dries » Unassigned
Category: bug » task
Priority: Major » Critical
Status: Closed (fixed) » Active
Issue tags: +Needs change record

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

cweagans’s picture

Assigned: Unassigned » cweagans

On it.

cweagans’s picture

Status: Active » Needs review

I updated all of the ones that xjm mentioned except for [#1626346]. That looks like a D7 entry. Does that need to be updated?

xjm’s picture

Priority: Critical » Major
Status: Needs review » Fixed
Issue tags: -Needs change record

@cweagans Right you are. Thanks for the quick turnaround!

Crell’s picture

ROFL! In my defense, dww, I still don't actually like YAML and in 2007 I was looking at Symfony 1. ;-)

ParisLiakos’s picture

Title: Change notice updates: Convert .info files to YAML » Convert .info files to YAML

restoring title

dww’s picture

Fixing the title for posterity and laughing back with Crell. :)

Thanks again, everyone...
-Derek

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

cweagans’s picture

Issue summary: View changes

Updating todo list and adding suggested commit message.

xjm’s picture

Component: base system » extension system
Issue summary: View changes
Issue tags: -Extension system