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

Purge the search tables directly #2265

Merged
merged 3 commits into from
Oct 6, 2020

Conversation

aschempp
Copy link
Member

@aschempp aschempp commented Sep 9, 2020

Q A
Fixed issues Fixes #2162

It does not make sense to delete from the search indexer(s), because we query the tl_search table directly. This can only work with the default indexer anyway.

@aschempp aschempp added the bug label Sep 9, 2020
@aschempp aschempp added this to the 4.10 milestone Sep 9, 2020
@aschempp aschempp requested review from Toflar and a team September 9, 2020 09:34
@aschempp aschempp self-assigned this Sep 9, 2020
@aschempp aschempp linked an issue Sep 9, 2020 that may be closed by this pull request
Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

Makes sense to me. 👍🏻 Let's wait for @Toflar's approval though.

Toflar
Toflar previously approved these changes Sep 9, 2020
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.

Fine for me, however, it was @ausi who built that when implementing the new search :)

@leofeyer leofeyer requested a review from ausi September 9, 2020 13:39
@ausi
Copy link
Member

ausi commented Sep 9, 2020

Shouldn’t we instead not query the tl_search table?
How should this work with a non-default indexer?

it was @ausi who built that when implementing the new search :)

I think I mainly switched to the indexer to remove the duplicated SQL code in 6fe7dc9 (#1679)

@leofeyer
Copy link
Member

Shouldn’t we instead not query the tl_search table?

How would we find the URL that belongs to the page ID then?

@aschempp
Copy link
Member Author

It doesn't work with other indexes. The (might) need to register their own callbacks.

@ausi
Copy link
Member

ausi commented Sep 10, 2020

How would we find the URL that belongs to the page ID then?

We could let the indexers themself decide what to delete?
The page ID can be passed via JSON LD?

$this->searchIndexer->delete(new Document(
	new Uri(''), 
	404, 
	[], 
	'<script type="application/ld+json">' . json_encode([
		'@context' => 'https://contao.org/',
		'@type' => 'Page',
		'pageId' => (int) $pageId,
	]) . '</script>'
));

@leofeyer
Copy link
Member

That is a clever idea for sure. 🦊

@Toflar
Copy link
Member

Toflar commented Sep 10, 2020

I propsed the same idea to @aschempp yesterday but there's just so much more that could possibly be added there. I can add my own JSON-LD data in the FE and use that to index. It would be missing here again.

@ausi
Copy link
Member

ausi commented Sep 10, 2020

The page ID is the only information we have in this case. If you added your own JSON-LD data, you can create a callback and call searchIndexer->delete() with your data the same way.

But this way we have the benefit, that the indexer can decide if and how it wants to handle the deletion.

@Toflar
Copy link
Member

Toflar commented Sep 10, 2020

I tend to agree, yes.

@aschempp
Copy link
Member Author

It might work. I'm just not sure the (massive) overhead is really worth the effort of theoretically have someone implement their own search index.

@Toflar and me also discussed that search results should use the same tags the reverse proxy does. This would allow to correctly purge the search from everywhere (e.g. when changing a news item).

@leofeyer
Copy link
Member

Ok, so how do we proceed? Are we going with @ausi's suggestion above or do I merge this PR?

search results should use the same tags the reverse proxy does. This would allow to correctly purge the search from everywhere

I assume that this would be a second step in a separate feature PR.

@aschempp
Copy link
Member Author

🤷 I would opt for my (simple) solution and hope for tags support in a future release …

@ausi
Copy link
Member

ausi commented Sep 28, 2020

Ok, so how do we proceed?

I think we should merge this PR for now and create a new issue for the suggestion from #2265 (comment)
If someone finds the time (and/or needs the feature) they can implement it.

@leofeyer leofeyer merged commit 4ff607a into contao:4.10 Oct 6, 2020
@aschempp aschempp deleted the bugfix/search-indexer branch October 10, 2020 14:56
AlexejKossmann pushed a commit to AlexejKossmann/contao that referenced this pull request Apr 6, 2021
Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | Fixes contao#2162

It does not make sense to delete from the search indexer(s), because we query the `tl_search` table directly. This can only work with the default indexer anyway.

Commits
-------

2f4bb81 Purge the search tables directly
97e747c CS
4033df8 Merge branch '4.10' into bugfix/search-indexer
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.

Non-existent service "contao.search.indexer"
4 participants