Clear invalid URLs using the new search indexer abstraction #887
Conversation
|
Thinking about it, why did we only ever clear on 404 so far? That doesn't make sense to me. Imho anything that is successful should be indexed and the rest should be cleared. |
|
Regarding the BC promise issue, maybe we should have some sort of contao/framework-bundle that is BC, and keep "implementation" stuff in contao/core-bundle that is not BC. |
Agreed |
Because there was no other way of knowing when a page does not exist any more. You either have to do a full reindex, which clears everything before hand, or you had to remove entries dynamically, when an URL throws a 404 exception. Not really sure though, how you would solve it now. |
|
Regarding the BC question, I would simply keep the name and add the functionality. Who cares if it does not exactly match, it's still our search index listener and it will be obsolete at some point 🤷♂ |
|
No idea why it would become obsolete. So shall I merge the logic into the existing |
|
As briefly discussed in Slack, renaming the class to I am aware that renaming the class is a BC break, but our BC promise does not cover event listeners. If we really care about this, though, we might as well keep the old listener and no longer use it. |
Toflar
force-pushed
the
feature/clear-search-document
branch
2 times, most recently
from
7b4a3fd
to
9b614ca
Compare
Toflar
force-pushed
the
feature/clear-search-document
branch
from
9b614ca
to
44fd9aa
Compare
I find it very difficult if you put out such statements. What's "our BC promise"? Nobody knows until it's documented so I started it over here: contao/docs#138. BTW: PR is updated, the failing tests are not related. |
|
|
||
| $this->indexer->index($document); | ||
| if (0 === \count($lds)) { | ||
| return; |
There was a problem hiding this comment.
Shouldn’t we also delete the document from the index in this case?
There was a problem hiding this comment.
I thought so too but that would result in useless search index delete commands for 99% of all the use cases (everything that's not coming from Contao in your own Symfony app). The 1% case would be where a page was once generated by Contao and it's now not anymore. I think this is very rare and sounds like you've fundamentally changed your app = you should rebuild the whole search index anyway.
There was a problem hiding this comment.
If search index delete commands for non-existent entries don’t hurt the performance too much, I think we should delete it in this case too to make it more robust.
There was a problem hiding this comment.
I really don't know about this. I still think we should keep it as is.
There was a problem hiding this comment.
I am also in favor of keeping the return statement. If there is no JSON data, the request is not a Contao request and the listener should not handle it.
There was a problem hiding this comment.
don’t hurt the performance too much
Think of an external search indexer like ES or Algolia. It's an API request for every response. Sure, it's a kernel.terminate listener but we still have users that do not use fpm or litespeed.
I will also submit another PR to be able to disable this listener completely (makes no sense to run it if you clean the search index regularly using a cronjob imho).
There was a problem hiding this comment.
The only unlogical thing about this is that we are not indexing the document if there is no JSON data but we are deleting it no matter whether there is JSON data or not. Is this intended?
Toflar
force-pushed
the
feature/clear-search-document
branch
from
a04776b
to
a2eb4d7
Compare
|
Thank you @Toflar. |
This is a follow-up PR for #730.
I've introduced a search indexer abstraction there but clearing a single URI was not part of it.
I've extended the interface which now also contains a
clearDocument()method (no BC break, 4.9 is still in development) and I've extracted the logic into its own listener so that it doesn't just work for our ownPageError404but for any exception.