API Update search functionality to work in a SearchContext #1323
Conversation
GuySartorelli
force-pushed
the
pulls/6.0/remove-elemental-search
branch
from
8b0fd1c to
89858aa
Compare
There was a problem hiding this comment.
We'll need to resolve silverstripe/developer-docs#705 (comment) first so we know what the scope of changes here needs to be
|
Following on discussion from silverstripe/developer-docs#705 (comment) There's a sliding scale of options for including element content as something that's included in general page search: A) Status quo - keep existing functionality, allow developers to opt-out when site grows and performance degrades I don't think C) is really viable given the lack of a drop in alterative, and because it's presumably very easy with our current knowledge to develop this, we may as well just do B) at that point rather than telling people to do it themselves. Between A) and B), I'd say we should probably still do A), because this being is done in the context of a refactoring card where the outcome should be that functionality remains the same while the under the hood code is now in a better state. I'm not liking the fact where just randomly dropping functionality which seems quite out of scope |
GuySartorelli
force-pushed
the
pulls/6.0/remove-elemental-search
branch
2 times, most recently
from
61f669c to
cd03de1
Compare
GuySartorelli
changed the title
|
I don't have time to argue unfortunately so I've just done what you want, though I still don't think it's the best course of action. Please next time advise you want these sorts of changes before merging the deprecation PRs - I have to go back and redo those now. |
| $pageIDs = []; | ||
| // The same extension can't be applied to the multiple classes in the same hierarchy | ||
| // without causing a host of problems, so we can be confident that we're not getting any double ups here. | ||
| $pageClassesWithExtension = ClassInfo::classesWithExtension(ElementalPageExtension::class, SiteTree::class, true); | ||
| // Get a list of classes that can't have elemental blocks despite having the extension to reduce the amount of | ||
| // records we're filtering through needlessly | ||
| $ignoredClasses = Config::forClass(ElementalPageExtension::class)->get('ignored_classes'); | ||
| foreach ($ignoredClasses as $class) { | ||
| $ignoredClasses = array_merge($ignoredClasses, ClassInfo::subclassesFor($class, false)); | ||
| } |
There was a problem hiding this comment.
This part and the exclude call are new, so we don't filter through records that just aren't relevant at all.
Everything else is the same which some minor refactoring to work in this method as opposed to the old one.
There was a problem hiding this comment.
I've merged the updated 5.4 PR - #1328
That will need to be merged-up, though I tried doing this myself though because of the recently merged change-tracker work it's a non-trival merge-up, you're best placed to resolve this, so could you please sort that, and then rebase this PR and run yarn build
This relied on the weird non-standard way CMSMain page filtering worked. This same functionality could be reintroduced by replacing the SiteTreeSearchContext with a custom one that filters by callback, but it won't be available in this module. Some improvements have also been made to avoid filtering through pages that don't have the relevant extension, and which are explicitly set to be ignored by the extension.
GuySartorelli
force-pushed
the
pulls/6.0/remove-elemental-search
branch
from
cd03de1 to
c2d90dd
Compare
|
Rebased. There's no JS or CSS changes in this PR so I didn't |
This relied on the weird non-standard way CMSMain page filtering worked. This same functionality could be reintroduced by replacing the SiteTreeSearchContext with a custom one that filters by callback, but it won't be available in this module.
Issue
CMSMain/HierarchyModelAdminsilverstripe-cms#2949