Problem

  • Contributed projects like Skinr and Libraries API use .info files to allow site builders to register plugins that can originate from other sources than modules or themes.
  • Those .info files are picked up by some Drupal core functions.
  • Functions in Drupal core that invoke drupal_system_listing() do not pass a regex that makes sure the basename of an .info file is a valid name that can later be used as PHP function prefix.

Goal

  • Allow projects like Skinr and Libraries API to keep on using .info files, but with a double-extension; e.g., SKIN.skinr.info, LIBRARY.library.info.

Details

  • "SKIN.skinr" is an invalid basename for modules and themes anyway. PHP functions cannot contain a dot/period. AFAIK, a valid basename for any Drupal system item must be in the form of [a-z][a-z0-9_]* anyway.
  • While it would make sense to rethink the function arguments of drupal_system_listing() based on the currently lacking enforcement of a valid basename in the calling code, that would be an API change, which can no longer be done for Drupal 7.
CommentFileSizeAuthor
#11 drupal.system-listing.11.patch4.93 KBsun
#3 drupal.system-listing.3.patch3.62 KBsun
drupal.system-listing.0.patch3.48 KBsun

Comments

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Needs work

AFAIK, a valid basename for any Drupal system item must be in the form of [a-z][a-z0-9_]* anyway.

This would be *way* too restrictive.

A PHP function name is [a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*. We already have this magical regular expression in _menu_router_build().

jacine’s picture

jacine
she/her
Location New York
commented

This is a big problem for Skinr. It causes packaged skin definitions (which are essentially micro themes) that reside in themes to appear on admin/appearance, like any other theme. Obviously the ability to enable a skin, that Drupal thinks is an actual theme is a disaster.

It's also a pretty silly bug in the first place. As @sun pointed out to me:

Add the following file:

themes/bartik/sub/some.theme!.info

...and Drupal will *try* to understand "some.theme!" as the internal, machine name of a theme, which is used to form PHP function names; i.e.:

some.theme!_preprocess_page()

Pretty funny, but not very smart of Drupal. ;)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal.system-listing.3.patch3.62 KB

Thanks -- changed to the correct pattern.

ChrisBryant’s picture

ChrisBryant commented

Subscribing, thanks for addressing this @sun!

jacine’s picture

jacine
she/her
Location New York
commented
Status: Needs review » Reviewed & tested by the community

Based on Damien's comment in #1, the patch in #3 good to me.

dries’s picture

dries commented

I understand the patch, and it looks good, I don't understand the environment in which this bug manifested itself.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

For example, the Skinr module instructs users/themers to put custom "skins", which are .info files, into their theme directory:

sites/all/themes/mytheme/skins/dropdown.info

dropdown.info contains meta data about a "Dropdown menu" skin, which can be applied to all kind of elements in a Drupal site to style them like a dropdown menu.

However, after putting that .info file into that location, users suddenly see a "Dropdown menu" theme on the Themes administration page.

To work around this problem, modules like Skinr, which are using custom .info files for custom meta data, should be able to use a double file extension of $plugin.skinr.info. Because that would lead to invalid basenames for Drupal core's default usage of .info files; i.e., a basename of something.skinr is neither a valid module name nor a valid theme name, so it should not be found and taken up by Drupal core in the first place.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

#3: drupal.system-listing.3.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

#3: drupal.system-listing.3.patch queued for re-testing.

owen barton’s picture

owen barton commented
Status: Reviewed & tested by the community » Needs work

The "[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*" looks like it would be really good to have as a constant (we can apply to the case in _menu_router_build() also).

Other than that, I think this looks great - the fact that you can currently register module that have unusable names is clearly a bug.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal.system-listing.11.patch4.93 KB

Good call.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Pattern has been moved into a constant. Any feedback?

owen barton’s picture

owen barton commented
Status: Needs review » Reviewed & tested by the community

This addresses #10, otherwise is functionally equivalent to #3 - looks good to me

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Nice clean-up. Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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