GLOB_BRACE not supported in some systems

[miklb]

Preface: issue reported by user sparkx on IRC, using Solaris, via Joyent hosting

Perhaps a fringe issue, but " The GLOB_BRACE flag is not available on some non GNU systems, like Solaris " http://us.php.net/glob

The only place this is used is in checking for screenshots on admin/themes. The error thrown is "Use of undefined constant GLOB_BRACE - assumed 'GLOB_BRACE' ../system/classes/adminhandler.php : Line 398", which relates to the get_all_data function in system/classes/themes.php starting on line 38.

I thought I had found a solution, but

//if GLOB_BRACE not available 
				if ( !defined("GLOB_BRACE") )
				{
				  if ( $screenshot = glob("$theme_path/*.jpg") );
				  elseif ( $screenshot = glob("$theme_path/*.jpeg") );
				  elseif ( $screenshot = glob("$theme_path/*.png") );
				  elseif ( $screenshot = glob("$theme_path/*.gif") );
				}
				else {
				  
				if ( $screenshot= Utils::glob( $theme_path . '/screenshot.{png,jpg,gif}' , GLOB_BRACE) ) {
					$themedata['screenshot'] = Site::get_url( 'habari' ) . "/" . dirname(str_replace( HABARI_PATH, '', $theme_path )) . '/' . basename( $theme_path ) . "/" . basename(reset($screenshot));
				}
				else {
					$themedata['screenshot'] = Site::get_url( 'habari' ) . "/system/admin/images/screenshot_default.png";
				}
				}

				self::$all_data[$theme_dir] = $themedata;
			}
		}
		return self::$all_data;
	}

didn't work in the users environment (via IRC, said blanked entire site), but did work in my test environment.

I suggest we find an alternate solution for this single use of the function, or figure out what I did wrong, as this would seem to open up more platforms.

[skippy]

The user is using Solaris, which is specifically an unsupported platform for the GOB_BRACE option.

The easiest solution is to say "All screenshots must be X", where X is a single image format.

Alternately, we could invoke several glob() calls, one for each supported image format.

Why, exactly, do we support three different image formats for the screenshots?

[itrebal]

I'd suggest adding something to theme.xml that defines the location of the screen shot, or having the location be statically defined.

[itrebal]

The code written above would allow any graphic through as long as it was in the theme's root directory; some themes may be minimalistic in regards to images, or have a poor sense of directory structure, and put their graphics in the root directory. We definitely need to limit it to screenshot.* if we're going to require them to follow a structure at all.

I'm for putting the screenshot's location into theme.xml, allowing the theme author to define any image they want, anywhere, of any type. This doesn't require any code on the back-end to deal with funny systems (ie: solaris) and allows more flexibility. Since the administrative system doesn't handle the graphics in any way except outputting a location for them, there isn't any solid reason not to allow any format as far as I can tell.

If nobody else is up for theme.xml, it should definitely be defined at a specific location. I would suggest requiring simply 'screenshot.jpg'.

[itrebal]

Sorry for the lack of a .diff - I can create one later if necessary. This patch is functional. I've not changed the functionality of the original code at all, just rewrote it to avoid GLOB_BRACE entirely.

public static function get_all_data() {

if ( !isset( self::$all_data ) ) {

$themedata = array(); foreach(self::get_all() as $theme_dir => $theme_path ) {

$themedatadir? = $theme_dir; $themedatapath? = $theme_path;

$themedatainfo? = simplexml_load_file( $theme_path . '/theme.xml' );

if ( $screenshots= Utils::glob( $theme_path . '/screenshot.*') ) {

foreach ($screenshots as $screenshot) {

if (preg_match('%/screenshot.(png|gif|jpg)$%', $screenshot)) {

$themedatascreenshot? = Site::get_url( 'habari' ) . "/" . dirname(str_replace( HABARI_PATH, , $theme_path )) . '/' . basename( $theme_path ) . "/" . basename($screenshot); break;

} else {

$themedatascreenshot? = Site::get_url( 'habari' ) . "/system/admin/images/screenshot_default.png";

}

}

} else {

$themedatascreenshot? = Site::get_url( 'habari' ) . "/system/admin/images/screenshot_default.png";

}

self::$all_data[$theme_dir] = $themedata;

}

} return self::$all_data;

}

[moeffju]

Replying to miklb:

if ( $screenshot= Utils::glob( $theme_path . '/screenshot.{png,jpg,gif}' , GLOB_BRACE) ) {

Why don't we just glob('screenshot.*')?

[lildude]

The fix for this is still not complete. After discussing with moeffju on IRC, changeset #1585 was created for part of the fix to ensure this takes effect on non-GNU systems correctly.

This then brought up two more issues:

- a typo on line 748 - problematic regex delimiters due to the use of '/' in file paths.

Attached is a patch that rectifies these two issues (for r1585) and should put this ticket to bed once and for all.

This patch, plus r1585 now works a treat on OpenSolaris.

[moeffju]

Thanks for the patch, but changing the delimiter will not be enough. The preg needs to be quoted with respect to the delimiters. Can you see whether r1586 fixes the problem? The pattern gets created correctly, quoted right, and the typo was fixed. In my lame simulation of a non-GLOB_BRACE-system, it worked fine.

[moeffju]

Confirmed fixed by lildude. Closing.