[#24150] - Code cleanup: remove setFieldAttribute('catid', 'action', …)
- Closed
- 26 Apr 2019
- Medium
- Build: staging
- # 24150
- Diff
- nhusung:fix-set-field-attribute-catid-action
- Success Hound No violations found. Woof! Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/drone/pr the build was successful Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
User tests: Successful: Unsuccessful:
Summary of Changes
Since the introduction of the categoryedit form field in the edit models/views of com_content, com_contact, com_newsfeeds and com_banners (d6673d7), there is no action attribute on the catid form field and thus no need to set it anymore.
Furthermore cleanups concerning the $id in ContentModelArticle::getForm() are performed. As the
getState('article.id') method returns the correct article id, no matter whether in frontend or backend, and seems to be the standard way of retrieving the article id, there is no need to fetch it from the CMSApplication's input.
Testing Instructions
As the changes are pretty small, a code review should be enough. However you can have a look at the category field inside the edit forms of com_content, com_contact, com_newsfeeds and com_banners with users associated to different groups and access levels and privileges.
Expected results
Same as before this pull
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_banners com_contact com_content com_newsfeeds |
| Labels |
Added:
?
|
||
| Title |
|
||||||
@HLeithner any Comment as Release Lead?
We need at least 2 tests and better testing instructions, also the merge conflict must be solved.
| Labels |
Removed:
J3 Issue
|
||
While I resolved the conflicts I tried hard to remove big parts the following code (as $form->setFieldAttribute('catid', 'action', '…') is not needed):
joomla-cms/administrator/components/com_content/models/article.php
Lines 392 to 420 in 30a4833
However, I’m not a hundred percent sure what @infograf768 intended in his last two commits (#24216 and #24640). To me it seems like issue #23991 occurs when a user with no edit.state permission on an already published article tries to change the article’s category. So why do we check for $id == 0 && !$user->authorise('core.edit.state', 'com_content')? If I’m not mistaking, this check is not relevant anyway. We already tested that $this->getState('article.id') is not 0 above. Furthermore, I’d like to question the $app->isClient('…') check. Couldn’t someone theoretically have permissions to view the article in backend but not to edit its state?
I could confirm my second concern (#24150 (comment)) so I just corrected it.
@HLeithner yes, the setFieldAttribute('catid', 'action', '…') lines are still present in J4. I already created #24329 which simply removes them. Sorry for the bad testing instructions but this is my first pull request ever and I was not quite sure what instructions to give when it can be verified from reviewing the code that the it does not do anything at all. Should I close this PR now?
thx and don't worry.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-04-26 10:19:59 |
| Closed_By | ⇒ | HLeithner |
As far as I know, many extensions developers use
com_contentas a template for their components. In order to stop developers from copying the unnecessary lines as fast as possible I decided to create a pull request on the staging branch. However, I started to wonder whether this is the correct place to put code cleanups (anyway it would not be merged into the 4.0-dev branch, would it?). That is why I created another pull request #24329.