Skip to content

Update data/generator/sfPropelModule/admin15/template/lib/helper.php #183

Merged
merged 1 commit into from Feb 14, 2013

3 participants

@e1himself

Fixed module generator Helper template.

Now it uses params['moduleName'] instead of params['route_prefix'] for url generation.
url_for supports module/action instead of route name to generate url.

All was fine when route_prefix was equal to moduleName. But in other cases we've got an error while generating url for moveUp and moveDown actions.

@jaugustin
Propel member

@e1himself could you add a test to prove the issue ?

@e1himself

Ok, I'll try

@willdurand willdurand merged commit 3af16a3 into propelorm:master Feb 14, 2013

1 check passed

Details default The Travis build passed
@willdurand
Propel member

thanks

@e1himself e1himself added a commit to e1himself/sfPropelORMPlugin that referenced this pull request Feb 17, 2013
@e1himself e1himself Implement functional testing for issue fixed by pull request #183 d0d194b
@e1himself

I tried to implement a unit test for this issue. But I've encountered some problems. The entire testing suit assumes that urlPrefix exactly matches module name and route prefix.

For example check test/functional/crud/crudBrowser.class.php:54

So you even unable test such cases with exising testing code.

Firstly, I've modified crudBrowser testing class to allow using it in conditions when module_name != route_prefix != url_prefix. Also I've added another route acme_article and crud11Test.php to ensure crudBrowser.php works fine with that (see 228eb65).

Then I've implemented actual test to reveal this issue #183 (see d0d194b):

  • Modified route to allow wildcards
  • Modified crud11Test.php to use admin15 generator theme
  • Modified ProjectConfiguration to generate also model filter classes (needded by admin15 theme)
  • Modified schema and added 'sortable' behavior to Artiicle model
  • Added crudBrowserAdmin15 class to test this issue (invalid links for 'Move up'/'Move down' actions)
@e1himself e1himself referenced this pull request Feb 17, 2013
Merged

Issue #183 test suite #188

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.