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

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

Why do issues automatically close? Seems to me they should only be closed manually when a solution is found and implemented.

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.

Yes, I understand that you provided a "work-around". But even so, I don't think something should remain in Drupal if it can crash the site as hard as this "bug" does. As I recall, the error message when it crashed wasn't even all that helpful in determining the cause. I had to spend way too much time retracing what I had done in order to pinpoint the cause.

If you can put forth a valid argument as to why an issue that crashes Drupal should be "closed", then I'm all ears.

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.

I see. Thanks for the clarification (Zen vs Drupal core). That makes sense. Hard to believe such a minor thing could cause such a hard failure in Drupal. Thanks for moving the issue to the proper place.

Cheers...

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:

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

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.

Thanks chx for the info on 7.x. Sorry to say though, I don't really understand this:

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.

Can you elaborate (in newbie terms)? Thanks.

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

The last submitted patch failed testing.

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 last submitted patch failed testing.

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

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

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.

The last submitted patch failed testing.

Setting to RTBC - testbot was broken.

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

The last submitted patch failed testing.

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

The last submitted patch failed testing.

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.