-
Notifications
You must be signed in to change notification settings - Fork 59
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
Made it easy to use PRG with just specific actions. #52
Made it easy to use PRG with just specific actions. #52
Conversation
It has been pretty annoying to repeat the action check over and over in the controllers. Let the component do this automatically for you now.
|
||
class PrgComponent extends Component | ||
{ | ||
|
||
/** | ||
* Default config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doc block could do with some explanation in my opinion about the actions
configuration setting. Due to it possibly containing bool
, string
or array
. From reading the code I assume you configure which controller actions you want the PrgComponent to run on, in which case wouldn't index
be the default as it's the most used use-case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not using index
as default because of backward compatibility. If you don't do the check manually in the controller it will apply to all actions by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it applies to all actions, edit
and add
actions using post will have their form data removed, which I think will be confusing for beginners.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree with that. This "WTF?" happened to me as well when I started using the plugin. IMHO you should have to explicit set the actions instead of the component processing all by default.
Now you can just configure the filters in the initialize() method of the table by calling the search manager instance directly. This finally makes it work the same way as other core features in models.
$controller = $this->_registry->getController(); | ||
$request = $controller->request; | ||
$this->controller = $this->_registry->getController(); | ||
$this->request = $this->controller->request; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->request
is already set by component's constructor.
@ADmad anything else? |
Should this not be explained in the readme too? |
👍 |
@burzum Looks good. Just the code eg. and related description for component in readme should be updated to use the new config. |
@ADmad done. |
/** | ||
* Checks if the current request has posted data and redirects the users | ||
* to the same action after converting the post data into GET params | ||
* | ||
* @return void|Cake\Network\Response | ||
* @return void|\Cake\Network\Response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should also be \Cake\Network\Response|null
if (is_string($actions)) { | ||
$actions = [$actions]; | ||
} | ||
if (in_array($this->request->action, $actions)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can just use return in_array($this->request->action, $actions);
Made it easy to use PRG with just specific actions.
👏 |
It has been pretty annoying to repeat the action check over and over in the controllers. Let the component do this automatically for you now.