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

Improve the "purge search results cache" label #1048

Merged
merged 1 commit into from Jan 13, 2020

Conversation

@Toflar
Copy link
Member

Toflar commented Dec 4, 2019

Fixed incorrect maintenance information now that we have a search indexer abstraction.

@Toflar Toflar added the defect label Dec 4, 2019
@Toflar Toflar added this to the 4.9 milestone Dec 4, 2019
@Toflar Toflar requested a review from leofeyer Dec 4, 2019
@Toflar Toflar self-assigned this Dec 4, 2019
*/
public function purgeSearchTables()
public function purgeSearchIndex()

This comment has been minimized.

Copy link
@leofeyer

leofeyer Dec 4, 2019

Member

Isn't the new method deprecated as well, because people should use the contao.search.indexer service?

This comment has been minimized.

Copy link
@Toflar

Toflar Dec 4, 2019

Author Member

We're only using this method to also purge the search cache folder. I could move that logic to the DefaultIndexer though. Wdyt?

This comment has been minimized.

Copy link
@leofeyer

leofeyer Dec 4, 2019

Member

If we do not need the new method then, I am for it. 👍

This comment has been minimized.

Copy link
@Toflar

Toflar Dec 4, 2019

Author Member

Agree 👍

'index' => array
(
'callback' => array('Contao\Automator', 'purgeSearchTables'),
'affected' => array('tl_search', 'tl_search_index')

This comment has been minimized.

Copy link
@leofeyer

leofeyer Dec 4, 2019

Member

I understand why you have removed this, but being able to see the number of entires in the tl_search tables was pretty helpful. Are you planning on re-adding it in some way in your crawler implementation?

This comment has been minimized.

Copy link
@Toflar

Toflar Dec 4, 2019

Author Member

We would need to implement some getNumberOfEntries() on the IndexerInterface. Not sure if that makes sense?

This comment has been minimized.

Copy link
@leofeyer

leofeyer Dec 4, 2019

Member

I am not sure if it makes sense on the interface, either.

I only know that being able to see the number of entries in the tl_search tables helped me see whether rebuilding the search index was successful and it helped me spot problems that caused the search index to grow too much. Therefore I would like to keep this information visible somehow.

This comment has been minimized.

Copy link
@Toflar

Toflar Dec 4, 2019

Author Member

Okay, I see. Good point. We do need to extend the interface then. Otherwise, if I disable the default search indexer and implement some Algolia search, this information is not helpful here, right?
I don't think you need to know the entries of tl_search and tl_search_index. A normal user doesn't know what these two tables are for anyway, right? I guess you only need them to see if there are any entries or not, correct?

This comment has been minimized.

Copy link
@leofeyer

This comment has been minimized.

Copy link
@Toflar

Toflar Dec 4, 2019

Author Member

Okay, let me come up with a proposal then :)

@Toflar

This comment has been minimized.

Copy link
Member Author

Toflar commented Dec 4, 2019

After working a bit on this I noticed that it does not make much sense to implement this globally. If I'm implementing a different indexer with e.g. Algolia I might want to show additonal information except for only the number of entries. I think we can agree that the two maintenance tasks are for the built in search and they don't have to be abstracted. I can easily unset them if I don't want them to be present.

I've improved the label though :)

@Toflar Toflar force-pushed the feature/maintenance-clear-search-index branch from 1229f5b to 19bfb7b Dec 4, 2019
@Toflar Toflar changed the title Adjust maintenance to new search indexer abstraction Improved purge search results cache maintenance label Jan 13, 2020
@leofeyer leofeyer merged commit 7448f2b into master Jan 13, 2020
9 checks passed
9 checks passed
Coverage
Details
Coding Style
Details
PHP 7.2
Details
PHP 7.3
Details
PHP 7.4
Details
Prefer Lowest
Details
Bundles
Details
Windows
Details
codecov/project 89.16% remains the same compared to 23630c1
Details
@leofeyer leofeyer deleted the feature/maintenance-clear-search-index branch Jan 13, 2020
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 13, 2020

Thank you @Toflar.

@leofeyer leofeyer changed the title Improved purge search results cache maintenance label Improve the "purge search results cache" label Jan 13, 2020
Tastaturberuf pushed a commit to Tastaturberuf/contao that referenced this pull request Jan 13, 2020
Description
-----------

Fixed incorrect maintenance information now that we have a search indexer abstraction.

Commits
-------

19bfb7b Improved maintenance label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.