mod_rewrite

replicant

Joined: 2005-01-04
Posts: 15
Posted: Tue, 2005-01-04 19:43

Ok,

I like how previously I could do the following:

http://www.websites.com/gallery/username

Now with gallery2 I have to do the following:

http://www.websites.com/gallery2/main.php/view/glenn/

So I tried just adding a simple mod_rewrite rule in Apache:

RewriteRule ^/gallery2/(.*) /gallery2/main.php/view/$1 [R]

Which gives me a "Redirection URL Limit" error.

Now is what I am trying to do supported in gallery2 itself, or do I need to continue mucking around w/mod_rewrite?

Thanks!

 
itcheg

Joined: 2003-11-30
Posts: 87
Posted: Tue, 2005-01-04 21:19

In Site Admin -> General there is a URL Style option where you can choose to use short URLs. that shoud help you some-what. If you want more than that you'll probably have to modify.

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Tue, 2005-01-04 21:46

I'd like to write a new module that supports mod_rewrite. It would create and maintain a .htaccess file with mod_rewrite rules and then tie into the url generator to create the proper short urls. I think that this is the right approach to take. If you, or somebody else, is interested in writing this module please let me know! I'll be happy to provide guidance...

 
replicant

Joined: 2005-01-04
Posts: 15
Posted: Tue, 2005-01-04 21:50

well, the "URL Style" option only formats the url like the following:

http://www.websites.com/gallery2/main.php/view/glenn/

I guess I take it I'll have to muck around w/mod_rewrite then?

 
nicokaiser

Joined: 2004-10-04
Posts: 118
Posted: Wed, 2005-01-05 01:01

Unfortunately I currently don't have that much time to dive into G2 module development and write some mod_rewrite module.

But I'd really really love to have such a module, and I don't think it's that difficult to create one, it needs some kind of ".htaccess creation function", a redirect mapping function and something that tells the url-builder in the core to build nice urls... (the first two steps may be in the Migration module...)

Maybe I have some spare hours during the next weeks... ;-)

 
chaos421
chaos421's picture

Joined: 2003-09-29
Posts: 44
Posted: Wed, 2005-01-05 02:24

instead of http://yoursite.com/gallery2/main.php/view/glenn/ why not use something shorter like http://yoursite.com/gallery2/x/glenn/ or http://yoursite.com/gallery2/view/glenn/ or something like that?

my $0.02,
keith

 
replicant

Joined: 2005-01-04
Posts: 15
Posted: Wed, 2005-01-05 04:33
chaos421 wrote:
instead of http://yoursite.com/gallery2/main.php/view/glenn/ why not use something shorter like http://yoursite.com/gallery2/x/glenn/ or http://yoursite.com/gallery2/view/glenn/ or something like that?

my $0.02,
keith

heh, either way .. can i do this within gallery2 itself, or am i going to have to much around w/mod_rewrite in a htacess or the .conf directly .. ?

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Wed, 2005-01-05 09:39

replicant, see my earlier post. I realize that wasn't totally clear -- but the point is that we (the dev team) want to write a module to maintain the .htaccess file for you and update the url generator appropriately. It's possible. Won't be trivial though.

 
pelle
pelle's picture

Joined: 2004-12-10
Posts: 389
Posted: Wed, 2005-01-05 18:14

Playing around with the idea of a mod_rewrite module I bumped into a small problem. What I've got so far is basically an empty extension of the GalleryUrlGenerator, just to see how things work. The way I've got the module to override the default UrlGenerator is by putting $gallery->setUrlGenerator($myGenerator); in performFactoryRegistrations(). The problem is that performFactoryRegistrations() is triggered after a bunch of URLs are allready created.

What is the proper way of overriding the GalleryUrlGenerator class?

 
replicant

Joined: 2005-01-04
Posts: 15
Posted: Wed, 2005-01-05 22:18
bharat wrote:
replicant, see my earlier post. I realize that wasn't totally clear -- but the point is that we (the dev team) want to write a module to maintain the .htaccess file for you and update the url generator appropriately. It's possible. Won't be trivial though.

if it's just for one URL it would be easy enough ;-) Or are ya'll wanting to make this an option for all albums in the gallery?

 
replicant

Joined: 2005-01-04
Posts: 15
Posted: Wed, 2005-01-05 22:36

also,

i would think this rule would work:

RewriteRule ^/gallery2/(.*)$ http://www.rensites.com/gallery2/main.php/view/$1 [R]

But it seems to go into a loop and gives me the "Redirection URL Limit" in my browser. Looking at the rewrite log, I get stuff like this:

[05/Jan/2005:16:35:08-0600][www.rensites.com/sid#3c08b544][rid#3c126034/initial](2)rewrite/gallery2/main.php/
view/main.php/view/main.php/view/main.php/view/main.php/view/main.php/view/main.php/view/main.php/view/
main.php/view/main.php/view/main.php/view/main.php/view/main.php/view/main.php/view/main.php/view/
main.php/view/main.php/view/main.php/view/main.php/view/glenn->/gallery2/main.php/view/main.php/view/
main.php/view/main.php/view/main.php/view/main.php/view/main.php/view/main.php/view/main.php/view/
main.php/view/main.php/view/main.php/view/main.php/view/main.php/view/main.php/vie

like it's some kind of loop. Are there any special considerations I need to take when writing my rules for use w/Gallery2?

[edited to insert line breaks]

 
pelle
pelle's picture

Joined: 2004-12-10
Posts: 389
Posted: Thu, 2005-01-06 00:23

Allright, I think I've got a way to use mod_rewrite and be somewhat compatible with Gallery1 url's.

This is what a rule could look like, ie:
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule ^([^\.\?/]+)/$ main.php/view/$1 [QSA]

As for the URLGenerator class I override two functions. First of all, getCurrentUrl(). This function originally uses $_SERVER['REQUEST_URI'] but what we realy need is the actuall adress since it's hidden behind the RewriteRule, thus falling back on the $_SERVER['SCRIPT_NAME']. Secondly, the new URLs needs to be shortened. I simply strip out /main.php/view/ from parent::generateUrl(...).

What it all comes down to in the end is that I dont know how to register my new UrlGenerator soon enough. As described in my erlier post many URLs are already created before my class is even registrated.

bharat, a little hint would be nice. Or maybe I'm way off base with thiss idea.

 
pelle
pelle's picture

Joined: 2004-12-10
Posts: 389
Posted: Thu, 2005-01-06 00:45

d'ho, bad RewriteRule. How about this?

RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule ^(.+)$ main.php/view/$1 [QSA]

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Thu, 2005-01-06 08:53

It looks like your rewrite rule at least partially works, so that's pretty cool. For experimentation purposes, I'd start by just modifying the core GalleryUrlGenerator until you get it working. Once we have something that works reliably, then we can worry about trying to extract it out into a module and register it properly. I'll look into that in the meantime...

 
nicokaiser

Joined: 2004-10-04
Posts: 118
Posted: Thu, 2005-01-06 10:51

pelle, these RewriteRules don't really work for me:

"/gallery2/album" instead of "/gallery2/main.php/v/album" does work, but then G2 considers this as root directory and uses "/gallery2/album/main.php/v/subalbum" for url generation (sub albums, photos, stylesheets, etc.)

I'm using Apache 2, maybe this makes the difference...

 
pelle
pelle's picture

Joined: 2004-12-10
Posts: 389
Posted: Thu, 2005-01-06 12:13

nicokaiser, right, thats why you need to make the UrlGenerator discharge $_SERVER['REQUEST_URI']. Edit /moules/core/class/GalleryUrlGenerator.class at line 131 where getCurrentUrl() starts. This is what the function should look like:

    function getCurrentUrl() {
	if (!empty($_SERVER['SCRIPT_NAME'])) {
	    $path = $_SERVER['SCRIPT_NAME'];
	    if (!empty($_SERVER['PATH_INFO']) && $_SERVER['PATH_INFO'] != $path) {
		$path .= $_SERVER['PATH_INFO'];
	    }
	}

	return $this->makeUrl($path);
    }

Now gallery realy knows where it's at.

Then edit line 621 (of the original file) where you should see a return statement like this:

return $url;

Make it look like this instead
return str_replace('/'. $this->_baseFile .'/view/', '/', $url);

This is what my .htaccess looks like:

RewriteEngine On

RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule ^(.+)$   main.php/view/$1   [QSA]

Make sure you have Short URLs turned on.

Edit:
bharat, I've got a moduel to handle the .htacces for me, a class that extends the UrlGenerator, all I need is to register the class now.

 
Einstein
Einstein's picture

Joined: 2003-10-13
Posts: 105
Posted: Thu, 2005-01-06 14:51

Could you also make this work for embedded systems. Like you are able to change the main.php.

Here an example
http://somesite.com/gallery/gallery.php/view/test/

 
pelle
pelle's picture

Joined: 2004-12-10
Posts: 389
Posted: Fri, 2005-01-07 14:43

Einstein, it should work with diffrent baseFiles. I haven't had the chance to test it though.

bharat,
The module only works if I edit the original GalleryUrlGenerator.class not to use $_SERVER['REQUEST_URI']. Some URLs (like the style sheet links and current view) are compiled before my module gets the chance to register its own UrlGenerator. With this fix the links get the right base, and correct view.

Fortunate enough, the album and file URLs are generated later on, with the new UrlGenerator.

When the module is activated URLs like http://e.com/gallery2/main.php/v/album/sub/ becomes http://e.com/gallery2/v/album/sub/ and http://e.com/gallery2/main.php/d/2691-1/picture.jpg becomes http://e.com/gallery2/d/2691-1/picture.jpg . This way any registered prefix works so that other modules can use mod_rewrite too.

Fix for the module to work:

in modules/core/classes/GalleryUrlGenerator.class at line 131:
replace getCurrentUrl() with this:

    function getCurrentUrl() {
	if (!empty($_SERVER['SCRIPT_NAME'])) {
	    $path = $_SERVER['SCRIPT_NAME'];
	    if (!empty($_SERVER['PATH_INFO']) && $_SERVER['PATH_INFO'] != $path) {
		$path .= $_SERVER['PATH_INFO'];
	    }
	}

	return $this->makeUrl($path);
    }
 
nicokaiser

Joined: 2004-10-04
Posts: 118
Posted: Fri, 2005-01-07 15:20

Works great! As I'm little of time right now, I didn't check if your GalleryUrlGenerator fix also works without the module -- but I guess this is something bharat knows.

One thing: sometimes G2 wants to redirect me, e.g. from ".../v/?g2_highlightId=3904" to ".../v/?g2_page=2", after the redirect there is main.php again... In the last 4 Minutes I couldn't figure out where these redirection takes place, seems like some class has to be informed what redirection urls look like.

 
pelle
pelle's picture

Joined: 2004-12-10
Posts: 389
Posted: Fri, 2005-01-07 18:46

nicokaiser.
My guess is that the module doesnt get the chance to set the new UrlGenerator before the redirect url is created.

If only here was a proper way of overriding the default class... or its me who have missed some function somewhere. Maybe bharat could point me in the right direction? Or anyone else of the G2 developers for that matter.

 
pelle
pelle's picture

Joined: 2004-12-10
Posts: 389
Posted: Sun, 2005-01-09 13:01

Here's an update on the project. I've fixed one bug in RewriteHelper::writeFile(), removed RewriteModule::update() since i cant trust apache_get_modules() and the only way to test if mod_rewrite works seems to be testing it in action.

 
nicokaiser

Joined: 2004-10-04
Posts: 118
Posted: Tue, 2005-01-11 00:06

rewrite 0.0.2 does not work for me (Apache 1.3.x, PHP 4.1.x), it displays a blank page when I try to install.

BTW, you are using "file_get_contents" which is not available in PHP < 4.3. This should be included to keep compatibility:

<?php
if(!function_exists('file_get_contents')) {
   function file_get_contents($file) {
       return implode('', file($file));
   }
}
?>
 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Tue, 2005-01-11 00:12

I'll do a full review of this tonight, but in the meantime...

You should actually be using:

$platform = $gallery->getPlatform();
$lines = implode('', $platform->file($file));

All filesystem related operations should be funnelled through GalleryPlatform to abstract us from the specifics of the underlying platform.

If you run the Php41Compatibility unit test, it will tell you about any other places where you used any functions that don't exist in PHP 4.1...

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Tue, 2005-01-11 06:53

This looks like an excellent start! I have a fair number of comments below.

General

  • We should probably create a new namespace for the urls we create because otherwise we'll wind up with possibly overlapping namespaces. For example, we have an actual file on disk called "themes/matrix/styles/theme.css". But we can also create a themes album, containing a matrix album, containing a styles album containing a file called "theme.css". The urls for the two would be exactly the same in your scheme and there'd be no way to differentiate them. I think that we should instead create a virtual namespace, like: http://your.server/gallery2/-/album1/album2/foo.jpg ..e by adding the /-/ we make sure that there's no conflict.
  • Missing unit tests. Their lack is really noticeable for RewriteUrlGenerator.class because it's hard to understand the intention of some of the code (like why you're removing the query string, etc) without seeing what edge cases are covered. All the controllers and helper methods should be tested.
  • Messing with the .htaccess file can lead to 500 errors on servers that don't want you to even create such a file. We need to provide a strategy to let users find out whether their server supports .htaccess, allows you to put stuff in it, and supports mod_rewrite. If we create a gallery2/.htaccess file on a server that doesn't support it then the whole app is dead in the water. The problem is that the only way to know for sure that it's going to work is to actually test it. In G1, we put a .htaccess file in the setup directory and actually tried it to see if it worked as part of the config wizard. However, this leads to the problem that on some subset of installations you get a 500 error just trying to use the config wizard. I think that a better approach is to create a series of directories under the test directory, each with a different .htaccess file in them. Then in the configuration page we provide links to those directories (using {g->url href="modules/rewrite/test/1"} etc) and we ask the user to click them. Or maybe even better we can have iframes inside the config view that loads them all for the user. If the page returns a 500 error then we know that whatever was inside that .htaccess file is invalid. If we set up the .htaccess files in those test dirs such that one of them has the whole feature set, the next one doesn't have Options, the next one doesn't have mod_rewrite, the final one has an empty .htaccess file, etc. then depending on which ones cause 500 errors we can tell what needs to be fixed on the server in order to get the feature working. And we'll know that we're not going to take down the user's gallery2 install if the webserver doesn't support .htaccess at all.

AdminRewrite.tpl

  • I'd rename some of the errors to be a little more descriptive (nofile => missing, blind => cantRead, protected => cantWrite).
  • 52: "hte" => "the"
  • 67,73: the <pre> tag should be indented to column 4
  • 70: Gallery2 -> Gallery

Htaccess.tpl

  • Missing standard boilerplate template comment
  • Is the Options line required for this feature to work? I'm wondering because I've seen servers where trying to change options will cause a 500 error.

RewriteUrlGenerator.class

  • 79: file_exists -> $platform->file_exists()
  • 74: operators should go on the line before the break (so move the "+" up from line 75)

RewriteHelper.class

  • getTemplateName() doesn't need to return a status code; it cannot generate a bad status. Also, internally to PHP you can always use forward slashes so you could just have it be a one liner:
    return dirname(__FILE__) . '/../templates/Htaccess.tpl';
    

    But then it's really not much more than a constant so I'm not sure that it's really necessary to have this method at all. I'll go on to explain how you can get rid of this method entirely.

  • in getTemplate() we should make Htaccess.tpl a real Smarty template instead of just a data file (like all the other .tpl files):
    $Htaccess = array();
    $Htaccess['baseFile'] = $baseFile;
    $template = new GalleryTemplate();
    $template->setVariable('Htaccess', $Htaccess);
    list ($ret, $htaccess) = $template->fetch('gallery:modules/rewrite/templates/Htaccess.tpl', 'modules_rewrite');
    if ($ret->isError()) {
      return array($ret->wrap(__FILE__, __LINE__), null);
    }
    

    Then in Htaccess.tpl, change "{basefile}" to "{$Htaccess.baseFile}" and that should be all. The advantage is that now if we choose we can use Smarty looping constructs, localization, etc all inside the template file. It will allow people to create their own local version of the template, etc. Note that the template system uses the relative path from the gallery2 directory, so you don't need to use RewriteHelper::getTemplateName() here. If you want to optimize this, you can store the results of the template into a static variable so that subsequent calls have no overhead.

  • all uses of file_get_contents() should be replaced by $platform->file() since file_get_contents requires PHP 4.3+
  • 64: "Cant" -> "Can\'t"
  • 83: substr(..., -24) is brittle. If we change anything, it'll break. Better to use dirname(dirname(dirname(__FILE__))), etc.
  • getFileName() is generic, how about getHtaccessPath() instead?
  • in checkFile(), see my comment about error names above. There's no need to use short names here. The longer the name, the more meaning it imparts and the less it leaves to imagination.
  • checkFile(): $orig is never used
  • checkFile(): I think a better API would be to return a true/false value to indicate whether the file is good or not, then return an additional value to indicate what is wrong with the file if you return false. This would simplify your check in writeFile() which right now has to check for the absence of 4 possible different errors. If you add a new error type to checkFile, that code will break. But if you use true/false then that code can just check the boolean.
  • writeFile(): $original is unused
  • writeFile(): should also return a success/failure boolean like I suggest for checkFile() to simplify error checking for its consumers.

AdminRewrite.inc:

  • 53,107: use isset() instead of !is_null() (positive logic is easier to read than negative)
  • 86: urlGenerator isn't used
  • the template container variable used in AdminRewriteView should be called $AdminRewrite. The canonical form is:
    $AdminRewrite = array();
    $AdminRewrite['file'] = $file;
    ...
    $template->setVariable('AdminRewrite', $AdminRewrite);
    

module.inc

  • 35: "Enabeles" -> "Enables"
  • 54: use isset() instead of !is_null() (positive logic is easier to read than negative)
  • 88: We need to come up with a better way for you to to do this. It's next on my list after I finish a couple more code reviews. This is fine for now, though.
  • 103: this line should be indented to 28 columns
  • I don't think that this module can be auto configured. See my general comment above on "test strategies" for a discussion of why.

[/]

[/]

[/]

[/]

[/]

[/]

[/]

 
nicokaiser

Joined: 2004-10-04
Posts: 118
Posted: Tue, 2005-01-11 07:29
bharat wrote:
We should probably create a new namespace for the urls we create because otherwise we'll wind up with possibly overlapping namespaces.
...
http://your.server/gallery2/-/album1/album2/foo.jpg

Doesn't the scheme create links like
"http://your.server/gallery2/v/album1/album2/foo.jpg"?
(Where "v" is the registered prefix, e.g. "d" for download).

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Tue, 2005-01-11 07:35
nicokaiser wrote:
bharat wrote:
We should probably create a new namespace for the urls we create because otherwise we'll wind up with possibly overlapping namespaces.
...
http://your.server/gallery2/-/album1/album2/foo.jpg

Doesn't the scheme create links like
"http://your.server/gallery2/v/album1/album2/foo.jpg"?
(Where "v" is the registered prefix, e.g. "d" for download).

Not quite. It creates URLs like this:

http://your.server/gallery2/main.php/v/album1/album2/foo.jpg

So main.php is passed "/v/album1/album2/foo.jpg" and it uses the first path element (/v) to determine what action it should take. This namespace can never conflict with anything inside the gallery2 directory because main.php can't be a directory AND a file.

In the solution that Pelle was proposing, we'd have a url like this:

http://your.server/gallery2/album1/album2/foo.jpg

Which could cause a conflict...

 
nicokaiser

Joined: 2004-10-04
Posts: 118
Posted: Tue, 2005-01-11 09:23

Well, the current version (0.0.2) does create URLs like
http://your.server/gallery2/v/album1/album2/foo.jpg

See http://siriux.net/photos/

Or did I misunderstand you?

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Tue, 2005-01-11 09:28

Oh..I misunderstood what you meant. In my inspection of the code I didn't see that it was generating urls with the /v prefix. My mistake!

 
pelle
pelle's picture

Joined: 2004-12-10
Posts: 389
Posted: Tue, 2005-01-11 16:28

Thanks for your input on this bharat.

As for the new namespace, as nicokaiser pointed out, it uses the existing /v prefix. My intensions are to make it configurable. If the URL links to an existing file or directory the RewriteUrlGenerator falls back to PathInfo.

Quote:
(like why you're removing the query string, etc)

When I check if the file exists, I need to strip it out.

Quote:
Is the Options line required for this feature to work?

The -Indexes is not requiered, but +FollowSymlinks are ( http://httpd.apache.org/docs-2.0/mod/mod_rewrite.html and http://httpd.apache.org/docs/mod/mod_rewrite.html).

 
pelle
pelle's picture

Joined: 2004-12-10
Posts: 389
Posted: Sat, 2005-01-15 22:17

Replying to myself... Well, its time for an update on the project. I've added ability to customize URL prefixes. You're not bound to 'v' or 'd' anymore. However, I still cant register my UrlGenerator in time which means that the only prefixes that actually works are 'view' and 'download' (other than 'v' and 'd').

What I haven't done yet is tests to see if mod_rewrite works. Nor have I made unit tests for my class and helper functions. The way I see it I'll fix the functionality first, then make tests to see if it works the way I want it to. Right now, it doesn't (you still need to hack the original core class).

Quote:
I think that a better approach is to create a series of directories under the test directory, each with a different .htaccess file in them. Then in the configuration page we provide links to those directories (using {g->url href="modules/rewrite/test/1"} etc) and we ask the user to click them.

This sounds like an exelent idea. Iframes is not valid xhtml though, and to load pages with php you need allow_url_fopen. I guess the best way to do this is letting the user click and discover if it works for themselves, or maybe a combination of the allow_url_fopen and the manual way. With allow_url_fopen I see a potential way to make the module auto configurable. How common is allow_url_fopen? Is it worth the hassle or should I stick with the manual click method?

 
nicokaiser

Joined: 2004-10-04
Posts: 118
Posted: Sat, 2005-01-15 22:57

pelle, just installed rewrite-0.0.4, works, but if I click "Save Settings" I get the following (settings seem to be saved however):

Quote:
Warning: mb_strrpos(): Empty haystack in /srv/www/siriux.net/htdocs-80/photos/modules/core/classes/GalleryUtilities.class on line 70

Warning: mb_strrpos(): Empty haystack in /srv/www/siriux.net/htdocs-80/photos/modules/core/classes/GalleryUtilities.class on line 70

Warning: Cannot modify header information - headers already sent by (output started at /srv/www/siriux.net/htdocs-80/photos/modules/core/classes/GalleryUtilities.class:70) in /srv/www/siriux.net/htdocs-80/photos/main.php on line 375

 
pelle
pelle's picture

Joined: 2004-12-10
Posts: 389
Posted: Sat, 2005-01-15 23:27

Hmm, that is strange. I've just updated to the latest cvs version and I dont get those warnings. With what data are you trying to save?

Edit: Think I've found it. You tried to save with an empty field, right?

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Sun, 2005-01-16 00:16

I'll review your latest code this evening. I've been working on refactoring the core so that we can do factory registrations more cheaply, and that'll allow us to set it up so that the url generator from your module has precedence. It'll take me another day or so to finish that up.

We can't count on allow_url_fopen -- it's the kind of feature that seems great but leads to endless potential security holes so it's generally disabled in most secure installs. However, you can use GalleryCoreApi::fetchWebPage() to do the same thing and it should be pretty reliable. If at some point in the future we find out that this approach doesn't work, we can always fall back on asking the user to click by hand.

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Tue, 2005-01-18 07:09

This is looking really good. It's very close to the point where you can commit it. You're still missing the unit tests, but as you said in your last post, you're going to do those later. Your code style is excellent; this module will blend right in with the rest of the code base. I'm thinking that when this module is in, it might make sense to ditch the PathInfo short url style altogether. This will be way more powerful and as long as we have this as an option we really don't need PathInfo!

Because of my recent API changes, I wasn't able to test the module. I hope to be able to test it next time around.

module.inc

  • I've refactored the way that we create the url generator so that now you can override the url generator properly. You need to make a few changes to bring things up to date:
    • Remove performFactoryRegistrations from your callback list in the constructor
    • Update your required core api to 2.1
    • Get rid of your performFactoryRegistrations method and put the following code into your upgrade() method:
          $ret = GalleryCoreApi::unregisterFactoryImplementationsByModuleId($this->getId()); 
          if ($ret->isError()) { 
              return $ret->wrap(__FILE__, __LINE__); 
          } 
      
          $ret = GalleryCoreApi::registerFactoryImplementation(
              'GalleryUrlGenerator', 'RewriteUrlGenerator', 'RewriteUrlGenerator',
              'modules/rewrite/classes/RewriteUrlGenerator.class', 'rewrite', null, 3);
          if ($ret->isError()) { 
              return $ret->wrap(__FILE__, __LINE__); 
          }
      

      then bump your module's version and once you upgrade, Gallery should be using your module's url generator instead of the one in core. I haven't had a chance to test this yet so you're going to be on the bleeding edge here (sorry!). Now the trick is to find a place where you can load your prefix map. This is tricky because the only methods that are going to get called are in RewriteUrlGenerator and none of those expect to send back GalleryStatus return codes. So for now, I'd move the code that loads the prefix map into RewriteUrlGenerator::init(), and if you get an error status back just swallow it and leave your prefix map empty and limp along without it. You could set an error bit and then just delegate all calls to GalleryUrlGenerator if there's an error init(), which would basically just take your rewrite class out of the equation.

RewriteUrlGenerator.class

  • The API for this class has changed, but it's simple. The constructor went away, and there's a new method called "init" that does everything the constructor used to. So change your constructor to:
        function init($baseFile, $relativeG2Path=null, $embedSessionString=null) { 
            parent::init($baseFile, $relativeG2Path, $embedSessionString); 
        } 
  • Now that you've got the url generator getting loaded in the right place, registerCustomViewPrefix() can go away or get rolled into init()

AdminRewrite.inc

  • as long as we're storing non-normalized data, why not store a serialized hash map? eg: $ret = GalleryCoreApi::setPluginParameter('module', 'rewrite', 'prefixMap', serialize($form['prefix']));
    Then in RewriteHelper::getPrefixMap:

     list ($ret, $map) = GalleryCoreApi::getPluginParameter('module', 'rewrite', 'prefixMap');
    // check $ret
    $map = unserialize($map);
    

    Less code, one less database lookup, unserialize is very fast, you don't have to worry about views/prefixes becoming de-synced.

  • Since you're only returning one error, it doesn't make sense to me to translate the nice descriptive constants into strings and then check them in the template as the strings. I think a better approach is to just keep them as constants all through the code. This means that in AdminRewrite you'd say:
    $form['error']['code'] = $error; then in AdminRewrite.tpl you'd have:

    {if isset($form.error.code)}
    {if ($form.error.code == REWRITE_STATUS_MISSING)}
    <div class="giError"> 
      {g->text text="The .htaccess file does not exist."} 
    </div> 
    {/if}
    ...
    {/if}
    

    . It's a little different from the normal pattern, but it's pretty readable.

RewriteHelper.class

  • The constants look great. I'd change them to be numeric though instead of string values (eg define('REWRITE_STATUS_MISSING', 1); etc)
  • 151: check the value of $ret
  • 155: the && should go on the line above
  • See my comment in AdminRewrite about simplifying getPrefixMap()

AdminRewrite.tpl

  • Strings like "Or let Gallery do the job for you. All you need..." are difficult to localize since sentence fragments depend on the context which isn't always obvious to the localizer. It'd be better to say something like "Gallery can do the job for you. All you need..."

[/]

[/]

[/]

[/]

[/]

[/]

 
pelle
pelle's picture

Joined: 2004-12-10
Posts: 389
Posted: Tue, 2005-01-18 19:13

Great! It works! However, first time around I'm gettin error on unregisterFactoryImplementationsByModuleId(). Guessing this is for upgrading the module, because it works when i comment this line out. I'm not ready to release any code yet. Need to finish up some added functions.

I dont think you should drop PathInfo, since not everyone has mod_rewrite, and the fact that RewriteUrlGenerator fallbacks back on PathInfo if the file exists.

 
mindless
mindless's picture

Joined: 2004-01-04
Posts: 8601
Posted: Tue, 2005-01-18 19:36

maybe PathInfo could be a separate module instead? then you can choose which short-url module you want to use...

 
nicokaiser

Joined: 2004-10-04
Posts: 118
Posted: Tue, 2005-01-18 19:41

@bharat:

in cvs: gallery/gallery2/modules/core/ShowItem.inc version 1.47 you wrote:

Quote:
Add the .html suffix to all short ShowItem urls so that the short tag (eg, .jpg) doesn't mislead other apps into thinking that it's not HTML

While you may be right with misleading apps, this leads to ugly URLs again:

/gallery2/v.html
/gallery2/v/myalbum.html
/gallery2/v/myalbum/picture001.jpg.html

How about switching to "path urls" again (without the ".html") but find a way to cut away the file extensions for pictures (e.g. on upload)?

/gallery2/v/
/gallery2/v/myalbum/
/gallery2/v/myalbum/picture001/

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Wed, 2005-01-19 02:06

I agree that thos urls are longer than they were before. But they're definitely more descriptive and we won't have conflicts this way.

If we preserve file extensions, then they have to go into the URL or if we have foo.jpg and foo.avi in the same album then /album/foo won't be enough for us to tell the difference. If we strip away the file extension, then we have to also do it to the file name *on disk* because currently we map the name of the file directly to the url.

The only alternate approach I can think of is to create a new field for every item which is a unique name that has no extension. So if you add "foo.jpg" to an album then its unique name would be "foo" and we could use that in the url. Then if you add "foo.avi" to the same album it might wind up with a unique name of "foo_001" or "foo-avi" or something. That would not be so hard to do, but it would be one more field that people would have to edit when they modify their items.

What do you think?

 
jmullan
jmullan's picture

Joined: 2002-07-28
Posts: 974
Posted: Wed, 2005-01-19 05:21

how about tying the item id in there for the viewing page?

/gallery2/v/
/gallery2/v/myalbum/
/gallery2/v/myalbum/1234.html
/gallery2/v/myalbum/picture001.jpg

where 1234 is the id of picture001.jpg?

Maybe not.

 
nicokaiser

Joined: 2004-10-04
Posts: 118
Posted: Wed, 2005-01-19 06:27

But the filename is already unique and editable, isn't it?
So what about

- strip extension from original filename
- if filename exists, add "-1", "-2", etc.

/gallery2/v/myalbum/foo
/gallery2/v/myalbum/foo-1
/gallery2/v/myalbum/foo-2
...

The problem with id's is, you can't delete and re-upload an image without changing its URL...

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Wed, 2005-01-19 07:45

If we strip the extension from the filename, then all the files on the filesystem won't have extensions and the only way you'll be able to tell that foo is a JPG and foo-1 is an AVI is to look in the database. That would drive me nuts. I want my file names to remain intact. I bet many others feel the same way.

So we could still do this, but we'd have to introduce a new unique identifier for each one so one album could contain:

FILENAME FILE ID
foo.jpg => foo
foo.avi => foo-1
foo.mov => foo-2

we can definitely do that. It's not that hard. In some ways it would be better because we'd detach the URL from the filename (and that connection causes us other minor issues). I'll let this simmer in the back of my head for a day or two and see if I can think of any refinements or issues with this.

 
nicokaiser

Joined: 2004-10-04
Posts: 118
Posted: Wed, 2005-01-19 07:57

This is what I meant - I was referring the "filename" field on the G2 admin form. So maybe "store files with their original names, *copy* filenames to the "filename" field (maybe rename it to "url name") and strip the extension" would be a solution.

 
nicokaiser

Joined: 2004-10-04
Posts: 118
Posted: Wed, 2005-01-19 12:36

Another idea:
is there a way for ShowItem to find out whether the current element can contain children or not? So we could just add ".html" to "leaf" elements:

/gallery2/v/
/gallery2/v/myalbum/
/gallery2/v/myalbum/dsc94719.jpg.html
/gallery2/v/myalbum/video1.mp4.html

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Wed, 2005-01-19 17:56

That's a good idea. I've changed it so that we only apply the .html to non-albums.

 
ryanduff

Joined: 2005-01-19
Posts: 2
Posted: Wed, 2005-01-19 22:15

I stepped out of the discussion on #gallery for a week or so, but I'm back now. Seems like things are getting juicy around here. The idea about the ID was good with /foo-1 and /foo-2 I think that'd work out good.

I did have one other suggestion for the rewrite and setting a base directory. I think it we could get rid of the /v/ easilly by doing this
1. It'd have to be an extra installation step though...
2. Have them install g2 into a g2 folder (ie. http://mysite.com/g2/)
3. Have the installer create a virtual folder at the root (easilly done via htaccess, ie. http://mysite.com/gallery/).
4. They can change it to whatever they want the gallery path to be as long as that directory doesn't reside at the root of their server.

If we make that known they can change it to, as I stated above, whatever path they wanted (ie http://mysite.com/mygallery/). This would also work great if the person wanted to run it right at the root as a gallery only site. Just a few thoughts to think about.

I'm going to hook up with the latest nightly tonight and the module from this thread and see how it goes. I'll think about what I said some more, maybe develop it more. Tell me what you think.[/b]

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Wed, 2005-01-19 22:41

Ryan, your suggestion will still have the same problem with conflicts if they have a root album name that coincides with some other directory in their document root. Ie, if they have a G2 album named "AlbumX" and they happen to have a directory called "AlbumX" then there's nothing that you can do in.htaccess to divine which link they really want.

But in general, I think that the rewrite module that Pelle is developing should be able to do this. After he gets it committed, we could tune it so that you can specify exactly where you want the .htaccess file to live and let you specify the base url for G2. This would allow you to install your G2 into http://your.server/gallery2/ but still be able to access it as http://your.server/, provided you have support for .htaccess

 
ryanduff

Joined: 2005-01-19
Posts: 2
Posted: Thu, 2005-01-20 13:01

bharat: I think you missed what I said. Let me explain better:

Physical site structure:
http://yoursite.com/g2/
http://yoursite.com/somefolder/
http://yoursite.com/somefolder/

Virtual site structure:
http://yoursite.com/gallery/
http://yoursite.com/gallery/album/
http://yoursite.com/gallery/album/image-1/
http://yoursite.com/gallery/album/image-1/large/
http://yoursite.com/somefolder/
http://yoursite.com/somefolder/

The only time this would interfere is if you also had a G1 installation which I believe, by default, is installed into /gallery/

The name of the example /gallery/ folder is changable and doesn't exist on the server. As long as there is no /gallery/ folder there is no conflicts. Same as any other rewrites.

Another thing, we could include some checking like with the file name, if theres a duplicate folder name for instance at the album level, it would make the id of the new album you're creating "album-1" thus aleviating any conflicts and narrowing down the only source of possible conflict to the root virtual directory.

 
pelle
pelle's picture

Joined: 2004-12-10
Posts: 389
Posted: Thu, 2005-01-20 16:18

This sounds like a great idea. In fact I've been thinking somewhat along those lines for new features, but first I want to finnish up the basic functionality.

Edit: I'd like to extend it to beond viewing core:ShowItem as in your example. For instance these virtual URLs:
http://yoursite.com/gallery/album/image1.jpg.html
http://yoursite.com/download/album/image1.jpg
This does not work if you want:
http://yorsite.com/album/imag2.jpg.html
Here you get the namespace problem too.

 
pelle
pelle's picture

Joined: 2004-12-10
Posts: 389
Posted: Fri, 2005-01-21 19:12

Here's the latest version.

Edit: I've realised that 0.0.5 not quite works. It generates some notices, so here's an update that should fix those.

bharat, when I run the core UrlGeneratorTest I get a bunch of failures. Is this normal?

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Wed, 2005-01-26 07:16

I'm in the process of reviewing this now. I have some preliminary notes for you:
General

  • Apache mod_rewrite test fails, but gives no indication of what is wrong.
  • The "Write File" button shouldn't appear if we can't write to the .htaccess file
  • Move the "Can not find Gallery data" message up to the Testing block so that it's clear that this is an explanation of why the "Gallery .htaccess" test failed
  • Clicking "test setup" gives me:
    Notice: Undefined index: write in /usr/home/bharat/public_html/gallery2/modules/rewrite/SetupRewrite.inc on line 53
    

    A controller test would have prevented this from slipping through.

The main problem I'm hitting (before even getting into the review) is that it doesn't work for me. The Apache mod_rewrite test generates a url like this:

http://me.com/g2/modules/rewrite/test/mod_rewrite/?match=/test/

But, when that gets rewritten, it doesn't have $_SERVER['PATH_INFO'] set, so it would appear that PathInfo doesn't work properly. Except that PathInfo *does* work properly for me if I enable short urls via the Site Admin. So I'm thinking that something about how mod_rewrite regenerates the URL is causing Apache2 not to pick it up correctly. I'm testing this with Apache/2.0.52 (Debian GNU/Linux) PHP/4.3.10-2.

I'm going to go read the mod_rewrite manual and see what I can come up with. If we can't get it working, then perhaps an alternative approach would be to provide a path= argument to core:ShowItem so that you can provide the path there instead of having to use PathInfo. I think that would be more reliable in general. What do you think?[/]

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Thu, 2005-01-27 01:24

I figured out why mod_rewrite isn't working for me. It's a problem in the test. I hardcoded the test to pass, and with some other minor tweaks it all works .. hooray!

The reason why the test doesn't work is that the RewriteRules won't work properly unless the RewriteBase is set correctly. We're not setting the RewriteBase, and without it my Apache2 setup isn't working. So I started tinkering with the code to figure out how to make it work.

The best thing that I could come up with is that we create a couple of test directories (test/mod_rewrite/gallery and test_mod_rewrite/gallery2) since /gallery and /gallery2 are the two most standard places that people will install the app. We can then check the current url (the URL generator will tell us this) and figure out if they've installed Gallery in either of those places, and if so then we can use that test dir. Each of those test dirs will have the appropriate RewriteBase line in it, eg:

RewriteBase /gallery2/modules/rewerite/test/mod-rewrite/gallery2

This works fine for me. Now the problem is, what if they install G2 into some other location or call it something different? Then neither of our prepackage directories are going to work. So in that case I think we should walk them through the process of letting us create our own test directory where we'll build our own .htaccess file with the appropriate line in it (we can re-use the existing Htaccess template with some modifications) and then let the user test that instead.

I'll take a whack at getting all of that working. Pelle, I don't mean to hijack your module here. Drop by #gallery sometime and we can chat about it in person.