:: in .info files causes Fatal error

Yes, that’s very annoying. The syntax of the .info file doesn’t allow you to put a :: in it. :-( [edit: this is a bug in Drupal core.]

Fortunately, there's a workaround. Use an HTML escape sequence for the semi-colon, :

settings[zen_breadcrumb_separator] = ' :: '

ExTexan, issues are automatically moved from "fixed" to "closed" after 2 weeks of inactivity. "Fixed" issues remain in the list of open issues so that other users can more easily see them, but "closed" ones can still be searched for.

I marked this issue "fixed" in comment #1 because I provided the solution.

I meant it wasn't a bug in Zen. But I'm happy to move it over to the main Drupal core issue queue.

Because it means having to re-write the way .info files are formatted, it won't ever be fixed in D6. And D7's code freeze is on Sept 1.

Looks like the issue occurs during this call in common.inc (while parsing the .info files), where Drupal checks to see if any of the values in the .info file might be constants:

// Handle PHP constants
if (defined($value)) {
  $value = constant($value);
}

Since PHP 5, the 'defined'-function can be used for class constants as well as 'normal' constants, and when it encounters '::' it expects a class + constant (it will also return a fatal error in case the class exists but the constant doesn't).

Attached is a patch that fixes this bug. Longer term, maybe we should consider using token replacements or something similar for the PHP constant replacements in the .info files.

The patch fetches a list of defined constants and checks against that list instead of using the 'defined' function. Besides no longer throwing fatal errors, some rudimentary tests show that this is about 40% faster than using defined (confirmation).

@ExTexan: sorry, this patch is for 7.x-dev. The usual approach is to first fix bugs in HEAD, and then optionally backport them to earlier versions. I don't have a copy of D6 but I'm guessing the approach will be similar: look for a function called drupal_parse_info_file and manually apply the edits from the patch.

Once this patch gets approved and commited, we can request that it be backported to D6.

More information about patches and applying them here: http://drupal.org/patch

I'm no expert, but if I were you I'd go with 6.x. This way if there is a bug, it's highly likely that you caused it yourself :) Additionally, you'll find very little production-ready modules for D7 at this point in time.

Also, if you are talking about developing modules or hacking core, take into account that 7.x is a moving target. APIs, functions, et al change very frequently, and some APIs might not yet be finished.

I wouldn't expect D7 to be released this year (here is some basic information about timings - apparently there are no roadmaps: http://www.optaros.com/blogs/drupal-acquia-and-2009)

Feel free to ask around on IRC or the forums if you want more opinions (I suggest we keep this issue on topic so we don't make reviewers read through pages of discussions about D7 releases :) )

Following some issue cleanup, I like the patch, but a) please use isset not array_key_exists b) write a test. I would write a test module and a text file and then var_exports the results of the parse into an unthemed page. The reason for this would be that fatal errors should not be tested in the process space of the parent Drupal but in the child.

ExTexan, I already unpublished several comments of yours on this issue. If you want to learn Drupal coding and test writing, I am eager to help you on IRC but not in this issue.

New patch attached:

• Changed array_key_exists to isset. Note that the result of array_key_exists and isset will differ for a constant set to NULL. Array_key_exists would return TRUE for a constant that is set to NULL, isset will return FALSE for the same constant. Is this an issue?
• Added a test that verifies that '::' does not throw a fatal error, that the value is parsed correctly, and that constant values still work.

The test was built according to chx's guidance (#13 and IRC). I originally used eval to parse the output of 'common-test/drupal-parse-info-file' back to a PHP variable, but using eval proved to be to cumbersome (and it can still take down the parent Drupal instance), not to mention that it's probably bad practice to use it anyway, so I've settled for plain text assertions instead...

Follow-up patch after talking to chx on IRC:

• Renamed the .info test file to .txt to make sure it won't be picked up as a genuine .info file;
• Used assertRaw instead of assertText;
• Removed print_r as var_export outputs by default.

Almost ready, but +class ParseInfoFilesTestCase extends DrupalWebTestCase { needs a doxygen.

Now with additional Doxygen thingy :)

I am so sorry for not catching this before but + $this->assertRaw("'simple_constant' => 6", t('Constant value was parsed correctly.'), t('System'));
this should use the constant instead of its value I think. "'simple_constant' => ". WATCHDOG_INFO

No problem at all. I choose WATCHDOG_INFO to have a constant that would be around for a long time and doesn't change much over time, but it didn't occur to me to include the same constant in the assertion. Doh.

New patch attached...

I like this patch very much. Is this our first fatal error testing? I think it is.

Hm. Can I ask why an entire module with a menu callback was created for this, rather than just checking the results of drupal_get_path('module', 'simpletest') . '/tests/common_test_info.txt'); directly in the assertions?

(Hint: that means we need to document why that is, if it's intended ;))

You can certainly ask :). I wondered about documentating this, but I have to admit that I don't know how to best phrase this... It boils down to something like this:

The extra module is necessary since a failed test might throw a fatal exception, halting further test processing. By moving it to its own module, the fatal error (if it occurs) is thrown in the child process instead of in the parent (testrunner) process, and it can be detected and handled cleanly without hindering other tests.

Here's chx's earlier quote describing the need for a separate module:

I would write a test module and a text file and then var_exports the results of the parse into an unthemed page. The reason for this would be that fatal errors should not be tested in the process space of the parent Drupal but in the child.

I think this way of working is similar to what is used in #469768: PASS.

Would this be ok as documentation at the start of common_test.module:

/**
* @file
* A test module to assist in testing common.inc.
* This module can be used whenever a test might potentially throw a fatal error. By
* moving those tests to this module, errors are generated in the child process and
* cannot hinder the overall (parent) test process.
*/

Re-rolled after changes to HEAD, and incorporated some documentation into common_test.module to explain the reasoning behind the use of a separate module.

The new comments look better. Couple of minor code style things:

+ * @file
+ * A test module to assist in testing common.inc.
+ * This module can be used whenever a test might potentially throw a fatal error. By

Should probably be a line break between the first line and the rest of the description.

+
+/**
+ * Implementation of hook_menu().

This is now "Implement hook_menu() in core.

Thanks, new patch attached with the code style changes mentioned in #30.

Looks good now, back to RTBC.

I'm not sure I understand why we create a new module for this. There is a lot of code that can throw fatal errors, and when that happens, we need to debug it either way. We improved reporting of fatal errors so this should be reasonably easy to do. Still feels like overkill to me.

Setting to RTBC - testbot was broken.

Setting to 'needs review' because of #33. I'm not convinced this is the proper approach.

Re-rolled. Dropped the extra module all-together in favor of just a unit test.

subscribing -- after submitting a duplicate issue in http://drupal.org/node/430166

This just killed contrib testing, lets get this in, eh? Re-roll and confirmation it worked on d6.

Stripped t() from getInfo().

D6 patch.

Cool. This approach makes a lot more sense than adding the overhead of a new module. And we can extend this tests in the text file to put other things that might break the .info file parser in the future.

Committed to HEAD! Thanks!

Marking back down to 6.x.

I'm not getting why we invoke constant() when we already have the value of the constant in an array key. Looks to be no reason for that.

@Gabor:

The original issue was defined() choking on keys with double colons in them, so the "check to see if we're dealing with a constant"-logic was altered, the rest (including actually loading the value of the constant) was left untouched.

From a DX perspective I actually prefer the current solution, performance-wise it looks like re-using the array is 2-3 times faster compared to loading the value via constant().

I don't really care whether we replace constant() or leave it in place... Patch attached in case looking up the value in the array is prefered over the use of the function.

If it's faster, let's do it...

Committed to CVS HEAD. Thanks!

Ok, let's port this updated approach to D6.

Merged #43 and #46 together.

This patch is running on scratch.drupal.org since we need it for some upcoming feature releases. Would be nice to see this land in d6.

Thanks, committed to Drupal 6 core, Translation template 6.x-3.x and Translation template 7.x-1.x where it was applicable.