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] Tagged Items Menu Item When Both Public and Registered Tags 404 #43922

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Aug 15, 2024

Pull Request for Issue #43920 .

Summary of Changes

removed unneeded check

Testing Instructions

  • Create a tag with access level Public
  • Create a tag with access level Registered
  • Assign public tag to an article with the access level Public
  • Assign registered tag to an article with the access level Registered
  • Create Menu Item Tags > Tagged Items
  • Access the frontend - do not login

Actual result BEFORE applying this Pull Request

404 Tag not found

Expected result AFTER applying this Pull Request

List with the public article

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

@brianteeman
Copy link
Contributor

This PR works BUT maybe there are other consequences of this changes as it was introduced by @Hackwar with #39114

In the tag model, the $item attribute wasn't defined. $tag at the same time wasn't used. With $item being initialised as an array, we can remove a bunch of checks. At the same time we can now properly throw a 404 if the number of tag IDs doesn't match the number of tags found in the database matching these IDs. (we might want to rewrite the TagModel::getItem() method to use a single query instead of loading each tag separately with a Table call...)

@MacJoom
Copy link
Contributor

MacJoom commented Aug 24, 2024

I have tested this item ✅ successfully on 798cbde

Tested with Joomla 4.4.7 PHP 8.1


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

@softforge
Copy link
Contributor

I have tested this item ✅ successfully on 798cbde

Does exactly as it should displaying only the public tag if not logged in and both if logged in


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

@dautrich
Copy link

I have tested this item ✅ successfully on 798cbde

Tested with J4.4.7 and PHP 8.3.6


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

@dautrich
Copy link

@softforge was faster than I was!

@richard67 richard67 changed the title [4] Tagged Items Menu Item When Both Public and Registered Tags 404 [4] Tagged Items Menu Item When Both Public and Registered Tags 404 Aug 24, 2024
@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 24, 2024
@Hackwar
Copy link
Member

Hackwar commented Aug 25, 2024

As @brianteeman already stated: I'm not sure that this is correct. The behavior of tags was never defined and is a big mess of different and competing concepts. Should it display all items which only match one tag or all tags? If a tag is not available to the current user, should the view display all items which match the other tag or would we still expect a filtered list with less items?

@alikon
Copy link
Contributor Author

alikon commented Aug 26, 2024

ok tags are a big mess, we can all agree

this pr fix a "common sense" behavior
not more not less

@laoneo laoneo merged commit b326bda into joomla:4.4-dev Sep 12, 2024
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 12, 2024
@laoneo
Copy link
Member

laoneo commented Sep 12, 2024

This really needs to be fixed, so I merged it. Thanks for the contribution.

@laoneo laoneo added this to the Joomla! 4.4.9 milestone Sep 12, 2024
@alikon alikon deleted the patch-22 branch September 12, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants