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

[RTM] allow multi root and multi page search #1462

Merged
merged 3 commits into from
Apr 27, 2018
Merged

[RTM] allow multi root and multi page search #1462

merged 3 commits into from
Apr 27, 2018

Conversation

fritzmg
Copy link
Contributor

@fritzmg fritzmg commented Apr 6, 2018

These changes allow you to expand/limit the search to multiple pages or website roots. This enables you to:

  • limit the search to multiple pages and its children - instead of just one page and its children
  • expand the search over multiple website roots - instead of being restricted to the current website root

The changes are fairly simple and straight forward. I tagged it as RFC since I am unsure whether the use of a new field called searchPages is really necessary. I decided to not use the existing pages field, since the sort order does not matter. Though may be I should simply alter the DCA in a load_callback, similar to tl_content::setSingleSrcFlags? Then the pages field could be reused for this.

@aschempp
Copy link
Member

aschempp commented Apr 9, 2018

Do I understand correctly that child pages of the selected ones are searched automatically? The field description does not mention that, maybe that would be useful?

Generally, I like the idea, have you tested whether the search result module also generates absolut links to the potential foreign domains?

@fritzmg
Copy link
Contributor Author

fritzmg commented Apr 9, 2018

Do I understand correctly that child pages of the selected ones are searched automatically? The field description does not mention that, maybe that would be useful?

Yes, child pages are searched automatically - as it's already the case in the current implementation when you select a reference page. The current field description doesn't mention it either:

Reference page

Please choose the reference page from the site structure.

However that field is shared with multiple modules, so the description has to be generic. We can choose a more descriptive one here, since it's its own field. Unless we switch to the pages field.

Generally, I like the idea, have you tested whether the search result module also generates absolut links to the potential foreign domains?

Yes :). This already worked anyway (if you set the reference page to a website root of another domain for example).

@leofeyer
Copy link
Member

I'm pro reusing the pages field if possible.

@Toflar
Copy link
Member

Toflar commented Apr 10, 2018

I'm pro reusing the pages field if possible.

Same here. Also I don't really think it is a problem if you can order pages. Who cares 😄

@fritzmg
Copy link
Contributor Author

fritzmg commented Apr 10, 2018

Alright, I'll update the PR accordingly :)

@fritzmg
Copy link
Contributor Author

fritzmg commented Apr 11, 2018

PR updated. I had to use a load_callback anyway, because the pages field is mandatory by default (which it should not be for the search module).

Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Excellent work!

@fritzmg fritzmg changed the title [RFC] allow multi root and multi page search [RTM] allow multi root and multi page search Apr 12, 2018
@leofeyer leofeyer added this to the 4.6.0 milestone Apr 13, 2018
leofeyer pushed a commit that referenced this pull request Apr 16, 2018
Description
-----------

While working on #1462 I noticed that `tl_modules.pages` has `'files'=>true` set in its `eval`. That does not make any sense, does it?

Commits
-------

aabf657 remove 'files'=>true for pages
{
switch ($dc->activeRecord->type)
{
case 'search':
Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessarily complex. Why not the following?

if ($dc->activeRecord && $dc->activeRecord->type == 'search')
{
    $GLOBALS['TL_DCA'][$dc->table]['fields'][$dc->field]['eval']['mandatory'] = false;
    unset($GLOBALS['TL_DCA'][$dc->table]['fields'][$dc->field]['eval']['orderField']);
}

BTW, is it really necessary to unset the order field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is unnecessarily complex.

True, it's unnecessarily complex. I used the same code as in tl_content::setSingleSrcFlags, in case more different changes would be necessary. I'll change it as suggested.

BTW, is it really necessary to unset the order field?

No, it's only for aesthetics 😁

@leofeyer leofeyer merged commit efaf96f into contao:master Apr 27, 2018
@leofeyer
Copy link
Member

Thank you @fritzmg.

leofeyer pushed a commit to contao/installation-bundle that referenced this pull request Jun 12, 2018
Description
-----------

Follow up PR of contao/core-bundle#1462

Commits
-------

3fc8a54 allow multi-root search
0096fa2 use tl_module.pages
@fritzmg fritzmg deleted the multiroot-search branch August 23, 2018 17:48
christian-kolb pushed a commit to christian-kolb/contao that referenced this pull request Oct 11, 2018
Description
-----------

Follow up PR of contao/core-bundle#1462

Commits
-------

3fc8a548 allow multi-root search
0096fa21 use tl_module.pages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants