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

[4.3] Allow category edit, although no category exists #40484

Merged
merged 3 commits into from
May 6, 2023

Conversation

bembelimen
Copy link
Contributor

@bembelimen bembelimen commented Apr 26, 2023

Issues: #40456

Summary of Changes

This is a very old bug which now becomes prominent with Joomla 4.3:

If you have no categories at all or only where you have no "core.create" access (as non-Super User), you can't create a new category if versioning is enabled:

grafik

The indirect reason is for the error is, that George did the right thing and uses the correct new Toolbar button call here: f05a062#diff-e7027c083e5734491c2543e3e20bce19f86291481b6ecdecf2e362b708b2351cL234 (PR: #39537). This leads to a strict requirement of having an integer as ID which throws this error, because new categories don't have an ID at all (NULL).

But the problem is now, when we create a new category, we should never reach this point, because some lines above there is a if/else construct: the if area should handle new categories and the else should handle editing categories (where our "version-line" from above is located).

So why are we reaching this point anyways when we have no other categories activated? Because there is the 2nd check in the if: (count($user->getAuthorisedCategories($component, 'core.create')) > 0). This says: you're only in creation mode when there is at least any other category, where you can create, too. Which makes no sense...

Testing Instructions

  • Run on PHP 8.1
  • Activate versioning in com_content (in the settings, it's activated by default)
  • Trash all categories (or remove all "core.create" permission in all categories when no Super User)
  • Create a new category

Actual result BEFORE applying this Pull Request

Error

Expected result AFTER applying this Pull Request

Creation works as expected

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

@richard67
Copy link
Member

@bembelimen To me it seems that this PR will solve issue #40456 . Am I right? if so, please mention the issue at the top of your PR's description and let me know so I can close the issue (or close it yourself). Thanks in advance.

@richard67 richard67 added the bug label Apr 26, 2023
@bembelimen
Copy link
Contributor Author

@bembelimen To me it seems that this PR will solve issue #40456 . Am I right? if so, please mention the issue at the top of your PR's description and let me know so I can close the issue (or close it yourself). Thanks in advance.

Yes, it does, thanks. Linked it and you can close it 👍

@richard67
Copy link
Member

@bembelimen Thanks.

@richard67
Copy link
Member

richard67 commented Apr 26, 2023

The code removed by this PR here could be a remainder from copy and paste from here: https://github.com/joomla/joomla-cms/blob/4.3-dev/administrator/components/com_content/src/View/Article/HtmlView.php#L146

For the articles this check makes sense, but not for the categories, so this PR here is right.

…View.php

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@richard67 richard67 added Small A PR which only has a small change Maintainers Checked Used if the PR is conceptional useful labels Apr 26, 2023
@chmst
Copy link
Contributor

chmst commented Apr 28, 2023

I have tested this item ✅ successfully on 7a7921d


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

1 similar comment
@joomdonation
Copy link
Contributor

I have tested this item ✅ successfully on 7a7921d


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

@joomla-cms-bot joomla-cms-bot removed Maintainers Checked Used if the PR is conceptional useful Small A PR which only has a small change labels Apr 28, 2023
@joomdonation
Copy link
Contributor

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 28, 2023
@joomdonation joomdonation removed the RTC This Pull Request is Ready To Commit label Apr 28, 2023
@richard67 richard67 added RTC This Pull Request is Ready To Commit Maintainers Checked Used if the PR is conceptional useful Small A PR which only has a small change labels Apr 28, 2023
@obuisard obuisard added this to the Joomla! 4.3.2 milestone May 6, 2023
@obuisard obuisard merged commit 329280b into joomla:4.3-dev May 6, 2023
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 6, 2023
@obuisard
Copy link
Contributor

obuisard commented May 6, 2023

Thank you Benjamin @bembelimen for this PR :-)

@robbiejackson
Copy link

@bembelimen thanks for this, which fixes a problem #40784 which I'd just noticed.

However, the call to getAuthorisedCategories occurs also in the categories view, in /administrator/components/com_categories/src/View/Categories/HtmlView.php in the lines (195-197 in joomla 4.3.1):

    if ($canDo->get('core.create') || count($user->getAuthorisedCategories($component, 'core.create')) > 0) {
        $toolbar->addNew('category.add');
    }

Tracing through the code of getAuthorisedCategories it looks for all that component's categories in the #__categories table, and gets the associated asset record.

It then calls authorise() on each asset record, and as this performs a recursive check up the asset hierarchy, so it ends up checking core.create against the component, which is the same as what is in the $canDo structure anyway.

So if my understanding is correct the call to getAuthorisedCategories is superfluous here, and if the user doesn't have core.create then it results in a lot of pointless database accesses.

Besides that, I think that the adding the New button should just be dependent upon having core.create permission anyway.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Maintainers Checked Used if the PR is conceptional useful PR-4.3-dev Small A PR which only has a small change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants