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] Several fixes to single tag view from com_tags #39113

Merged
merged 11 commits into from
Nov 29, 2022

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Oct 31, 2022

Summary of Changes

This is the result of a codereview of the tag view from com_tags. There are several changes in here, which fix broken behavior in this one view and clean up code. I'm doing my best to document everything in here... This is a complex change, but unfortunately I don't see how to simplify this and/or split this up into several PRs. For all existing sites, this PR will not result in new errors, but rather fix existing issues.

Let's start into the changes in here:

  • The view class has been reordered to mimic the article view of com_content. This means, that data is assigned directly to a class attribute and not to a temporary variable, after the data is collected from the model, the parameters are merged appropriately and then the plugin events are run.
  • A bunch of docblocks have been fixed.
  • The feature to count hits on tags has been moved from the view to the controller, similar to how it is done in com_content. An option to disable this has been added as well.
  • $this->item in the view class contains an array of tags and up till now only the first tag was checked against the menu item, etc. Now all the tags are checked.
  • At several places, the view class checked the access levels. This is something that we are doing in the model (already) not in the view.
  • Merging of parameters was unnecessarily complex.
  • 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...)

Testing Instructions

Basically there should be no difference on an existing site between the current code and after applying this PR.

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

Hackwar and others added 3 commits October 31, 2022 16:43
Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Brian Teeman <brian@teeman.net>
@chmst
Copy link
Contributor

chmst commented Nov 2, 2022

Then I misunderstood that, sorry. I see the problem from your point of view. But as a user I would be ok to know that there is nothing to see. Without 404 error message.

@jamfx
Copy link

jamfx commented Nov 3, 2022

I have tested this item ✅ successfully on 80c6c27

Testes and no Problems found.


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

@Hackwar Hackwar mentioned this pull request Nov 9, 2022
@richard67
Copy link
Member

I've restored @jamfx 's test result in the issue tracker since the commits which have invalidated the test count were just clean branch updates.

@Hackwar Please avoid unnecessary branch updates since that invalidates the test counter in the issue tracker and so makes it harder for us to find the PRs which have 2 good tests and causes additional work for restoring the test result. Thanks in advance.

@jwaisner
Copy link
Member

I have tested this item ✅ successfully on 47f9758


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

@jwaisner
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 22, 2022
@cyanama
Copy link

cyanama commented Nov 27, 2022

I have tested this item ✅ successfully

Please give us customizable css styles for the image and the title. At the moment you need to customize the raw img and h1 statements with impact for the whole webpage.

thanks
Cyana


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

@richard67
Copy link
Member

RTC is still valid because the change after that was just replacing 3 nested if statements by one with the combined condition, which is ok for me by review.

@obuisard obuisard added this to the Joomla! 4.3.0 milestone Nov 29, 2022
@obuisard obuisard merged commit 97e6593 into joomla:4.3-dev Nov 29, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 29, 2022
@obuisard
Copy link
Contributor

Thank you Hannes @Hackwar for the PR!

@Hackwar Hackwar deleted the 4.3-tags-1 branch November 30, 2022 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants