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

Add check get category #258

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zonky2
Copy link

@zonky2 zonky2 commented Feb 24, 2024

With the customisation, other hooks can also customise the list.

@fritzmg
Copy link
Contributor

fritzmg commented Feb 25, 2024

You can also give your Hook a higher priority.

@zonky2
Copy link
Author

zonky2 commented Feb 25, 2024

You can also give your Hook a higher priority.

I set the other hook (https://github.com/numero2/contao-tags) a priority = 100 - without the PR I see
Uncaught PHP Exception Symfony\Component\HttpKernel\Exception\NotFoundHttpException: "Unused arguments: category" at /html/contao4/vendor/contao/core-bundle/src/EventListener/ExceptionConverterListener.php line 97

see numero2/contao-tags#6

zonky2 added a commit to zonky2/contao-tags that referenced this pull request Feb 25, 2024
Set priority for newsListFetchItems for better collaboration with news categories

see also codefog/contao-news_categories#258
@fritzmg
Copy link
Contributor

fritzmg commented Feb 25, 2024

Yes, your own Hook will then need to process the category. Or what exactly is your goal?

@zonky2
Copy link
Author

zonky2 commented Feb 25, 2024

There are two different news listings - one list can be filtered by category, the other by tags.

This does not work with the previous implementations.

@fritzmg
Copy link
Contributor

fritzmg commented Feb 25, 2024

This needs to be fixed differently then. If category filtering is not enabled, then the hook needs to return false.

But you will also have to make sure, that category URLs do not point to the wrong news list.

Copy link
Contributor

@fritzmg fritzmg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, this cannot be merged as is, as there are a few problems:

  • The category parameter is hardcoded, instead of being fetched from the manager.
  • This would break the standard filter functionality (i.e. filtering by categories, even without an active category).
  • Input::get($categoryParam) should probably not be executed here without the 3rd parameter being true.
  • Only newsListFetchItems was adjusted, instead of both hooks, leading to inconsistent behaviour.

But as already mentioned, the actual solution needs to be implemented completely differently. In essence the the hooks need to return false if only the standard Contao behaviour applies (or should apply).

@aschempp
Copy link
Collaborator

does the hook in news-categories need to return false, or some other place?

@fritzmg
Copy link
Contributor

fritzmg commented Feb 26, 2024

does the hook in news-categories need to return false, or some other place?

Yes - basically if $criteria = $this->getCriteria() only adds the basic criteria and nothing else then the hooks need to return false. Not sure how this could be done while keeping the same structure. May be add something like NewsCriteria::isOnlyBasicCriteria() and then keep track of that in the other functions?

@fritzmg
Copy link
Contributor

fritzmg commented Feb 26, 2024

@aschempp @qzminski if you agree, I can try a PR.

@aschempp
Copy link
Collaborator

I have no opinion, but feel free to give it a try 😅

@fritzmg
Copy link
Contributor

fritzmg commented Feb 26, 2024

See #263

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants