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

EZP-32105: Use search.pagination.limit parameter instead of obsolete pagination.search_limit #12

Conversation

kmadejski
Copy link
Member

Question Answer
Tickets EZP-32105
Bug fix? yes
New feature? no
BC breaks? yes/no
Tests pass? yes
Doc needed? no
License GPL-2.0

Checklist:

  • Coding standards ($ composer fix-cs)

@kmadejski kmadejski added Bug Something isn't working Ready for review labels Oct 26, 2020
@kmadejski kmadejski changed the base branch from master to 1.0 October 26, 2020 15:09
@kmadejski kmadejski force-pushed the EZP_32105_SearchType_in_ezplatform-search_uses_obsolete_configuration_parameter_for_pagination_limit branch from dab24db to f87409e Compare October 26, 2020 15:09
@kmadejski kmadejski force-pushed the EZP_32105_SearchType_in_ezplatform-search_uses_obsolete_configuration_parameter_for_pagination_limit branch from f87409e to 081f3cd Compare October 26, 2020 15:11
Copy link
Contributor

@ViniTou ViniTou left a comment

Choose a reason for hiding this comment

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

But can we also get additional PR to adminUI with deprecating old configuration?

Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

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

I think the parameter has to be changed here as well:
https://github.com/ezsystems/ezplatform-search/blob/1.0/src/lib/View/SearchViewFilter.php#L76

otherwise I had exceptions when accesing the search on other SiteAccess than site (for example: example.com/pol/search )
With the change I've mentioned above it works for me, but only if I define:

parameters:
    ezsettings.site_group.search_view:
        full:
            default:
                template: "@@ezdesign/search/index.html.twig"
                match: []

(similar as https://github.com/ezsystems/ezplatform-search/blob/master/src/bundle/Resources/config/default_settings.yaml#L4 but for site_group where my SiteAccesses are).

Q: If I define the view for default scope (ezsettings.default.search_view) then the template for AdminUI is overridden as well and AdminUI search stops working - is it a misconfiguration on my side/a bug in the product somehow related to scopes?

@kmadejski
Copy link
Member Author

Thanks, @mnocon! I have fixed this another occurence you've found. Could you please re-test?

Q: If I define the view for default scope (ezsettings.default.search_view) then the template for AdminUI is overridden as well and AdminUI search stops working - is it a misconfiguration on my side/a bug in the product somehow related to scopes?

I adapted it to site_group as this solves the problem for a clean installation. Your observation is indeed very interesting though, as AdminUI should (re)define this template on its own (I guess for admin_group) and therefore this default configuration should not interfere with it. If it behaves differently then we might have a bug, even so it is not directly related to this PR.

Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

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

@kmadejski looks good now!

I've reported the other thing I've mentioned in a separate ticket: https://jira.ez.no/browse/EZP-32121

@lserwatka lserwatka merged commit 77f53bf into 1.0 Oct 29, 2020
@lserwatka lserwatka deleted the EZP_32105_SearchType_in_ezplatform-search_uses_obsolete_configuration_parameter_for_pagination_limit branch October 29, 2020 10:26
@lserwatka
Copy link
Member

You can merge it up.

@kmadejski
Copy link
Member Author

@lserwatka done in a9d2574.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7 participants