Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cakephp fashioned redirect if possible #7

Closed
wants to merge 6 commits into from
Closed

cakephp fashioned redirect if possible #7

wants to merge 6 commits into from

Conversation

cap5lut
Copy link
Contributor

@cap5lut cap5lut commented Sep 20, 2017

Sorry that I am bothering you again.

Due some debugging in my cakephp project I realized how the redirecting of this plugin works and that its levering out the cakephp's redirect. So I thought of adding that.

Basically it will just check if there is a redirect method in the controller and passes the redirect attribute to it, because in the cakephp's way to pass the redirection data (for example as array("controller" => "tests", "action" => "index")) I changed the initialization of $this->redirect and the comparison in the redirectIfIsSet method as well.

If the redirect method does not exist it will still use the old way.

@AlessandroMinoccheri
Copy link
Owner

Thanks for your contribute!

Can you please add one or more tests for this?

@cap5lut
Copy link
Contributor Author

cap5lut commented Sep 21, 2017

Oh right, I always forget to add tests, sorry, will do it in ~1h

@AlessandroMinoccheri
Copy link
Owner

No problem, when you can.

It's important to add test when you add a feature to demonstrate that your code works fine, It's not a very used plugin but tests guarantee that this plugin works fine if someone would like to use It.

Thanks for your contribute

@cap5lut
Copy link
Contributor Author

cap5lut commented Sep 21, 2017

Some unexpected stuff occured, so its delayed, and I have a question: for the test i basically need another class, so where should I put it? (Basically its just a class that implements redirect, to check afterwards if its redirect method gets called)

@AlessandroMinoccheri
Copy link
Owner

You can a directory into test like "Resources" and put there a class for example

@cap5lut
Copy link
Contributor Author

cap5lut commented Sep 21, 2017

Something is still wrong with the namespacing for the MockRedirect class, can you help me there?

I've put the class into the file /tests/TestCase/Resources/MockRedirect.php
The namespace is App\Test\TestCase\Resources
The test itself uses use App\Test\TestCase\Resources\MockRedirect;

But it still won't load the class ending up in an class not found error.

@AlessandroMinoccheri
Copy link
Owner

Can you show me the error?

Try to launch the command:

composer dump-autoload

@cap5lut
Copy link
Contributor Author

cap5lut commented Sep 22, 2017

well as Scrutinizer check says:

There was 1 error:

1) App\Test\TestCase\Controller\Component\UserPermissionComponentTest::testCakePhpFashionedRedirect
Error: Class 'App\Test\TestCase\Resources\MockRedirect' not found

/home/scrutinizer/build/tests/TestCase/Controller/Component/UserPermissionComponentTest.php:235

Do I have to run composer dump-autoload in the CakePHP root directory or in the plugin root directory?

@AlessandroMinoccheri
Copy link
Owner

Try to change autoload into composer.json into this:

"autoload": {
      "psr-4": {
          "UserPermissions\\": "src",
          "Tests\\":"tests"
      }
  }

And after launch composer dump-autoload into the root of the project and into the root of the plugin please

@cap5lut
Copy link
Contributor Author

cap5lut commented Sep 23, 2017

It's still showing the same error.

@AlessandroMinoccheri
Copy link
Owner

If you can't include into Resources directory, try to put the class into the same directory of the test file please @cap5lut

@AlessandroMinoccheri
Copy link
Owner

Any news on this PR? @cap5lut

@AlessandroMinoccheri
Copy link
Owner

I am closing this PR because it passed a lot of time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants