[5.4] Better code for save2copy Smart Search Filter #46449
Conversation
|
I have tested this item ✅ successfully on 8640a46 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46449. |
|
I have tested this item ✅ successfully on 8640a46 I just want to be specific if the Filter is called: BUT if you have an Title that ends in a number like: This happens the same in other areas of Joomla so it is not an issue with this PR, that's why I validated the test. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46449. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46449. |
|
so good This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/46449. |
| $task = $app->getInput()->get('task', '', 'cmd'); | ||
| if ($this->getState('task') === 'save2copy') { | ||
| $table = $this->getTable(); | ||
| $table->load(Factory::getApplication()->getInput()->getInt('filter_id', 0)); |
There was a problem hiding this comment.
The filter_id id should be passed from the controller's save function to here as there is the input object available.
There was a problem hiding this comment.
@laoneo For that, we should find a way to handle it in save method of FormController https://github.com/joomla/joomla-cms/blob/5.4-dev/libraries/src/MVC/Controller/FormController.php#L575 instead of having to do that in every controller. As this is a bug fix release, I don't want to touch the our MVC code.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes. I can do that. But it is just a workaround. Later (in 6.1 for example), we might decide to pass ID of the source the record uses a state variable instead of via $data array, and in that case, we will need to update the code here again.
There was a problem hiding this comment.
It is not a workaround, the controller is where you should deal with user input.
There was a problem hiding this comment.
I know. But as I said, we should find a way to handle it in FormController so that we have to pass that data in each child controller.
There was a problem hiding this comment.
Anyway, I will stop doing change until we have a final decision. Look like maintainers want to revert the original PR and fix/handle it properly in 6.1
|
I'm closing this PR because maintainers decided to revert the original PR and implement a better/proper solution for 6.1 |
Pull Request for Issue # .
Summary of Changes
This is a follow PR for #46081. There are few problems with that PR:
Testing Instructions
Actual result BEFORE applying this Pull Request
Works, but code is complicated than it should
Expected result AFTER applying this Pull Request
Works, with simpler to maintain code
Link to documentations
Please select:
Documentation link for docs.joomla.org:
No documentation changes for docs.joomla.org needed
Pull Request link for manual.joomla.org:
No documentation changes for manual.joomla.org needed