Description: ------------ I am currently implementing a Datagrid component in the Seagull framework (seagullproject.org). I use a search engine friendly url strategy resulting in urls like: http://dev.mydomain.lan/datagrid/action/try1/ I use the Pager Renderer for paging the datagrid. To match the urls used in the framework I had the options to configure the url like in CODE SNIPPET 1. I am wondering if any plans exist for implementing this sort of configuration options in the Datagrid Renderer strategy. I can imagine something like code snippet 2. In combination with a few functions like in code snippet 3. Seagull among other frameworks/CMS empties the $_REQUEST and $_GET which are now used to interpret the request and generate sorting urls. The url generation I simply solved by subclassing the HTMLTable renderer. But manually inserting values into the $_GET and $_REQUEST can't be the way to go? Any response is appreciated : ) Test script: --------------- // CODE SNIPPET 1 $tableOptions = array( // other options 'sortingResetsPaging' => false, // set request url without 'selfPath' => SGL_URL::makeLink('try1', 'datagrid', 'datagrid', array(),''), 'fileName' => '/orderBy/%o/direction/%d/page/%p/' ); // Output table and paging links $test = $datagrid->render('SGLHTMLTable', $tableOptions); // CODE SNIPPET 2 $options = array( 'pagerOptions' => array( // other options ... 'append' => false, 'fileName' => '/page/%d/', // To get Seagull URL Style working for Pager 'path' => 'http://dev.mydomain.lan/datagrid/datagrid/action/try1/', 'currentPage' => $req->get('page'), ) ); $test = $datagrid->render(DATAGRID_RENDER_PAGER, $options); // CODE SNIPPET 3 $datagrid->setCurrentPage(); $datagrid->setDirection(); $datgrid->setCurrentPage();

Let's clarify a few things first. You say: > I use the Pager Renderer for paging the datagrid. > To match the urls used in the framework I had the > options to configure the url like in CODE SNIPPET 1. Don't you mean "CODE SNIPPET 2"? > I am wondering if any plans exist for implementing > this sort of configuration options in the Datagrid > Renderer strategy. I can imagine something like > code snippet 2. Don't you mean "CODE SNIPPET 1"? Couldn't you please post on http://phpfi.com (or somewhere else) the driver you wrote by subclassing the HTMLTable renderer? That would help us to understand your need. Btw, your request subject wasn't complete. I allowed myself to change it.

There is already the method _getRequestArgument, the simplest solution for this would be to add a new Method, for example "setRequestParameterCallback($callback)". If a callback is set, _getRequestArgument would call this callback with $name as parameter instead of directly access $_POST|GET|COOKIE. I can create a patch if it is not clear, what I mean.

Hello Sascha and Fortunato, Do we really need a callback? Wouldn't a global printf format string do the trick? Example: $datagrid->setRequestFormat("/page/%d/sort/%s/%s"); This pattern would be used with sscanf() in _parseHttpRequest(), and with sprintf() in renderers. You would call setRequestFormat() as an alternative to setRequestPrefix() and all renderers would take it into account automatically. It would allow some variations: $datagrid->setRequestFormat("/sort/%2$s/%3$s/page/%1$d"); $datagrid->setRequestFormat("/page/%d"); In the later case (which only include the page number), the GET arguments would still be generated/parsed for orderBy and direction arguments. The only problem is that printf format strings are quite limited in comparison to regular expressions, but unfortunately there's no such thing as printf with regex. In the Django framework they have a such reverse URL resolver, but it would need to be ported from Python to PHP. What's you point of view?

Yes, that should work, it is even better if it is also used to generate the URL's. I am not sure if it would be possible to define parts of an URL, so that /controller/action/page/1 would map to /page/%d. Imho, this would make the generation of URL's very complex. On the other side, an "absolute" request format has to be set on every page, but this shouldn't be that difficult if you use a framework like seagull.

Well, actually scanf is pretty simple, if you parse "/controller/action/page/1" with "/page/%d", it will work and extract the page number. However the reverse isn't true, printf would generate "/page/1". It's also true that, in general, with that sort of "clean" urls, you use absolute ones, whether you're under Seagull or another framework. And if someone really wants to use some sort of relative path configuration, they can do something like setRequestFormat("$mypath/page/%d"); Otherwise, I think you're right, if we allow relative path such as "path/%d", it would make things rather complex and bug-prone. Anyway, it's something we could add in the future, if some users really need it. I see that you proposed to make a patch in your initial post. Would you like to try and implement setRequestFormat() and post a patch in here?

(maybe that setUrlFormat() is a better name btw)

I can try to work something out, but it will take a few days.

That sounds good. Before you implement it, here are a few remarks: In the future we might support other parameters, in addition to page, orderBy and direction. An example is "search". In this regard the %s and %d expressions might get obscure. So the following might be better: setUrlFormat("/page/{page}/{orderBy}/{direction}") I think the { and } caracters are safe to use. They should almost never appear in URLs. Ideally they could be escaped in the expression, but we can support that in the future. For now, instead of scanf and printf, to support such keyword based expressions, you'll need to use some simple strings replacements and regex. That shoudn't be a big deal though. However, the regex should be as restrictive as possible. For example, when parsing the page number, only allow for numeric input. Another point: SDG support sorting by multiple fields. That sounds a little bit tricky, and might make the expression complex. I propose we do not support multiple fields sorting in clean urls for now. It will still be possible to pass them via GET arrays.

I have created a patch against Structures_DataGrid.php 1.137, which does only the parsing. It is a first version, and not very well tested. Some hints: * active it with: $sdg->setUrlFormat('/.../{page}/{orderBy}/{direction}'); * everything behind the format is ignored but it has to start from beginning * parsing itself is very flexible, all possible parameters are stored in an array with format 'name' => 'regex' * regex is quoted with preg_quote, so it should be possible to use every character inside the URL format * the regex runs only once per call * a second regex is needed to get the order of the params. (/{page}/{orderBy} is different from /{orderBy}/{page}. I am not sure if there is a faster way to get this information, I did not found one.. * orderBy validation regex is wide open ((.+)), I am not sure what should be allowed there and what not. Would be easier with TODO #2, I think.. * multiple sort is not possible at this time but should not be too complex. I could think of something like /sortColumn1;sortColumn2/ASC;DESC/. Again, TODO #2 could make this easier I am open for some feedback... :)

I haven't tested it yet, but reading your patch it looks like excellent work ! A few remarks: * some people might want to use both url format and GET/POST/COOKIE (GPC) args (for example with the HTMLSortForm renderer). Example: http://foo.bar/page/1/?orderBy=foobar. So please don't completely bypass the GPC parsing when there's a urlFormat as you're currently doing. * _parseUrlFormat() doesn't have a meaningful name. It uses the format but doesn't parse it. It's more about "url argument" or similar IMO. * setUrlFormat() doesn't reset $_urlParsedParams. This may be a hidden bug, but if for some reason the user calls this method twice, the second call is likely to have no effect. * please don't do too much error testing as the is_string() test in setUrlFormat(). This is somehow heavy. > * orderBy validation regex is wide open ((.+)), I am not > sure what should be allowed there and what not. Would be > easier with TODO #2, I think.. It ok for now. It might require some testing with various URLs though. Anyway, it's not dangerous because we quote SQL identifiers in the datasources. But any work on TODO #2 would be greatly welcome. > * multiple sort is not possible at this time but should > not be too complex. I could think of something like > /sortColumn1;sortColumn2/ASC;DESC/. Again, TODO #2 could > make this easier This requires some more thinking. I don't think TODO #2 is relevant here. I propose we leave this point for later. Thanks for your contribution

> * some people might want to use both url format > and GET/POST/COOKIE (GPC) args Changed, so that it goes on if nothing was found with urlFormat. > * _parseUrlFormat() doesn't have a meaningful name. changed to _getUrlFormatArgument, according to _getRequestArgument > * setUrlFormat() doesn't reset $_urlParsedParams. > * please don't do too much error testing as the is_string() test > in setUrlFormat(). This is somehow heavy. Ok, changed. > > * orderBy validation regex is wide open ((.+)), I am not > > sure what should be allowed there and what not. Would be > > easier with TODO #2, I think.. > It ok for now. It might require some testing with various > URLs though. > Anyway, it's not dangerous because we quote SQL identifiers > in the datasources. But any work on TODO #2 would be greatly welcome. I see one problem with that so far. If {orderBy} is at the end of the UrlFormat, it is not possible to add something behind. For example with /page/{page}/{direction}/{orderBy} and the URL "/page/2/ASC/sortColumn/?someOtherParam=1", orderBy will become "sortColumn/?someOtherParam=1"

A few questions about the generating.. * How should paging links without an explicit sorting look? UrlFormat is quite hardcoded, so /page/5 does not map to /page/{page}/{orderBy}/{direction}... Multiple UrlFormats? dummy values for orderBy/direction/..? Some way to make parts of the UrlFormat optional, like "/page/{page}(/{orderBy}/{direction})" ? * How should the url format be forwarded to the renderers? Perhabs with the new common renderer options feature? ($this->setRendererOptions($urlFormatOptions, true) * How should it work with HTMLTable-Renderer? Just extending "_buildSortingHttpQuery" is not enough, because the ? is hardcoded somewhere else (see line 387 of HTMLTable.php). Perhabs this part could be moved to _buildSortingHttpQuery...

Hello Sascha, Sorry for the late answer, I have a lot of work to finish before going on vacations on august. Meanwhile, I discovered there's a package for both parsing and generating URLs : http://pear.php.net/package/Net_URL_Mapper I'm sorry I didn't know about it before. I think we should use it because it follows the same well-know syntax used both the python route package and Ruby on Rails. It is not documented, however the mentioned packages are. Here's the best startpoint to documentation I know of: http://routes.groovie.org/ Additionally, the Net_URL_Mapper unit tests are rather nice: http://cvs.php.net/viewvc.cgi/pear/Net_URL_Mapper/tests/ The main drawback is that it's PHP5 only. Mark and I discussed about it, and we think it's ok as long as SDG remains a PHP4 package and that the url-mapping feature is only available on PHP5. That might get a little tricky, especially if you need to handle exceptions, because try { } catch statements cause syntax errors in PHP4, but I believe you shouldn't need to. Otherwise you might need to load the url-mapping routines from an external file. Regarding you questions, I think that once you use Net_URL_Mapper, the situation is likely to be very different, and the solutions too. Once again sorry for the rewrite that all of this might need. As I told you I'm going on vacations very soon. So maybe that Mark will have time to help you if you have any questions.

> Hello Sascha, > > Sorry for the late answer, I have a lot of work to finish before going > on vacations on august. No problem, same here... > > Meanwhile, I discovered there's a package for both parsing and > generating URLs : > http://pear.php.net/package/Net_URL_Mapper Yes, I know Net_URL_Mapper. I am already using it in a project, but didn't mention it because of the PHP4/5 thing. I had a few problems with the generate-feature and it is quite "heavy/slow" but I think the integretation should be possible and useful. It should solve most of the problems I had, like multiple URL's for parsing AND generating, optional parts and so on. > The main drawback is that it's PHP5 only. Mark and I discussed about > it, and we think it's ok as long as SDG remains a PHP4 package and that > the url-mapping feature is only available on PHP5. > > That might get a little tricky, especially if you need to handle > exceptions, because try { } catch statements cause syntax errors in > PHP4, but I believe you shouldn't need to. > Otherwise you might need to load the url-mapping routines from an > external file. We could simply say that the exception has to be catched by the developer. So, the exception goes untouched "through" the php4 compatible code. If someone with PHP4 calls setUrlFormat, he would get a fatal error on the first php5 only keyword inside net_url_mapper, should this be checked and handled with a PEAR_Error? > > Regarding you questions, I think that once you use Net_URL_Mapper, the > situation is likely to be very different, and the solutions too. The first question is solved, but there is still that hardcoded '?' and the renderer needs to know about the urlFormat.

> Yes, I know Net_URL_Mapper. I am already using it in a > project, but didn't mention it because of the PHP4/5 > thing. That's good news. It's a good thing that you already know the package :) > I had a few problems with the generate-feature and it > is quite "heavy/slow" but I think the integretation > should be possible and useful. To me, the strength of NUM (Net_Url_Mapper) is the fact that it follows a well know url-mapping syntax. That will decrease the learning curve for SDG newbies. If it's buggy or heavy, you might want to post some bug reports to the NUM maintainer, Bertrand, and try to contribute. > If someone with PHP4 calls setUrlFormat, he would get > a fatal error on the first php5 only keyword inside > net_url_mapper, should this be checked and handled with > a PEAR_Error? No, NUM depends on PHP5 so the PEAR installer won't install it for PHP4 users. But I recommend you simply check for the availability of NUM in setUrlFormat(), and issue a PEAR_Error if it's not there (no need for PHP version checking), like we do for datasources and renderers. > The first question is solved, but there is still that > hardcoded '?' and the renderer needs to know about > the urlFormat. These issues need some investigation and I don't have the time currently. Please do what you consider to be right. I trust the way you code.

Finally, I've made a new version with Net_URL_Mapper. It consists of 3 seperate patches * Datagrid.php: Loading, configuring of NUM and parsing the request * Renderer.php: A new method for building a URL with NUM * HTMLTable.php, Pager.php, Smarty.php: Usage of NUM if aviable (HTMLTable and Pager tested, Smarty untested) Usage: try { $dg->setUrlFormat('/page/:page/:orderBy/:direction'); } catch (Net_URL_Mapper_InvalidException $invalid) { die('Bad URL!'); } Features: * orderBy and direction are optional, if they are not set, they are ignored (/page/2 and /page/2/field/ASC are both valid and are correctly parsed) * Support for NUM prefix and scriptname options. Setting a prefix makes it possible to use multiple Grids on the same page. However, only one grid can be sorted/paged at one time * Support for additional GET/extraVar Parameters, which are submitted with a normal GET-String (actually, this is a NUM-Feature) TODO: * Multipe path definitions. This is fully supported by NUM, but not yet implemented, as I am not sure how useful it would be here... * Support for multiple sorting fields. This would need a definition, how they are submitted (perhabs with a delimiter like , oder ;) * Other renderers? * everything I forgot :)

Thank you for this excellent work. Your patches have been commited on CVS. Regarding the TODO's you mention, I propose we let them aside for now, and wait for users to post bugs/requests according to their needs. URL mapping is rather complete now IMO. I'm closing this bug.