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

Publish task of JControllerAdmin or AdminController reports some errors as success #17808

Closed
ggppdk opened this issue Aug 31, 2017 · 5 comments
Closed

Comments

@ggppdk
Copy link
Contributor

ggppdk commented Aug 31, 2017

Steps to reproduce the issue

i do not have steps but it happens in cases of 2 error in the publish task of (J3.7.5)JModelAdmin or (J3.8+)AdminModel
JLIB_APPLICATION_ERROR_EDITSTATE_NOT_PERMITTED
JLIB_APPLICATION_ERROR_CHECKIN_USER_MISMATCH


[EDIT] short explanation
publish task , call model->publish , which return without setting error via setError
publish task look at getError, see none, and assumes success

Also faster to understand is to look at this code lines:
https://github.com/joomla/joomla-cms/blob/staging/libraries/src/MVC/Controller/AdminController.php#L211-L224

https://github.com/joomla/joomla-cms/blob/staging/libraries/src/MVC/Model/AdminModel.php#L1005-L1024

And also in then menus controller identical code as (J3.7.5)JControllerAdmin or (J3.8)AdminController
https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_menus/controllers/items.php#L224-L238


Detailed Explanation of the issue
The publish method of (J3.7.7)JModelAdmin or (J3.8)AdminModel

  • for the errors mentioned above it will return false without setting an error by calling $this->setError()

Instead the error is added to the log file via JLog

Looking at the code it looks like

  • the intention was to exclude the ID given the error and continue with changing the state of the rest of the record IDs, but ... then it execution continues returning false without calling setError()

Then back at the publish() control task of (J3.7.5)JControllerAdmin or (J3.8)AdminController, it does not check if model->publish() returned false !

  • instead it checks if there were errors via $model->getError(), and since no error Messages were added it assumed that model->publish() was a success

Expected result

There model errors are show as failure by the publish() controller task
JLIB_APPLICATION_ERROR_EDITSTATE_NOT_PERMITTED
JLIB_APPLICATION_ERROR_CHECKIN_USER_MISMATCH

Actual result

They are shown as success

System information (as much as possible)

J3.7.5 / J3.8 staging

Additional comments

I think the intention of the code since the bad $PK is being unset,
was to allow the publish task at controller to decide if it will show the error or continue and call the model->publish task again, or something like this

An example of this happening with publish task of menus controller is here (note that menus controller has same code as (J3.7.5)JControllerAdmin or (J3.8)AdminController)

#15938 (comment)

@ggppdk
Copy link
Contributor Author

ggppdk commented Aug 31, 2017

It is easy to fix this , just a decision needs to be made

  • either the model will set an error message thus the publish task will see the error Message and report it
  • or the error message will not be set and the publish task at controller will check for false and show failure without a message (this is not good option)
  • or the publish task get the error, check for which pks will be removed, and enqueue an error about the bad record ID, and continue to call model->publish again to publish/unpublish the remaining record IDs ?

@ggppdk
Copy link
Contributor Author

ggppdk commented Aug 31, 2017

I have edited description to add links to the code, now it will be easy to see what is happening

@ghost
Copy link

ghost commented Nov 25, 2017

Status chnged to "Needs Review".


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17808.

@gwsdesk
Copy link

gwsdesk commented Dec 9, 2017

I tend to opt for the third "dot" option

@brianteeman
Copy link
Contributor

Thank you for raising this issue.

Joomla 3 is now in security only mode with no further bug fixes or new features.

As this issue doesn't relate to Joomla 4 it will now been closed.

If we are mistaken and this does apply to Joomla 4 please open a new issue (and reference this one if you wish) with updated details for testing in Joomla 4.
cc @zero-24

@zero-24 zero-24 closed this as completed Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

5 participants