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

Index configuration overrides DO subsite field if NULL value #68

Conversation

mark-a-j-adriano
Copy link

Background:

DataObject does not have a SubsiteID field and is not being indexed even if explicitly defined in the Subsite Config.

Added Solution:

  • Allow indexing of DO even if SubsiteID is not defined (null)
  • Allow DO to be added on multiple Subsite search Index
App\Model\Product: &product_defaults
          fields:
            title:
              property: Title
            last_updated:
              property: LastUpdated
            code:
              property: Code
              options:
                type: text

@mark-a-j-adriano mark-a-j-adriano force-pushed the feature/explicit-dataobject-for-subsite branch 2 times, most recently from 8da5321 to c500f1d Compare May 13, 2022 11:44
Copy link
Contributor

@andrewandante andrewandante left a comment

Choose a reason for hiding this comment

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

Some minor tidy ups but the idea makes sense - I'd like some sort of documentation around this as well, so if there's space in the ReadMe to add a line that says something like:

"If you are using subsites, but your object doesn't have a subsite ID, it will be included inthe index if it is explicitly added, e.g. "

Then that would be awesome

src/Extensions/Subsites/IndexConfigurationExtension.php Outdated Show resolved Hide resolved
src/Extensions/Subsites/IndexConfigurationExtension.php Outdated Show resolved Hide resolved
@mark-a-j-adriano mark-a-j-adriano force-pushed the feature/explicit-dataobject-for-subsite branch 4 times, most recently from c8e8a61 to 7a55f29 Compare May 17, 2022 10:53
@mark-a-j-adriano mark-a-j-adriano marked this pull request as ready for review May 17, 2022 10:54
@mark-a-j-adriano mark-a-j-adriano force-pushed the feature/explicit-dataobject-for-subsite branch 2 times, most recently from 1561526 to 4a414c1 Compare May 18, 2022 05:46
Copy link
Author

@mark-a-j-adriano mark-a-j-adriano left a comment

Choose a reason for hiding this comment

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

Admin change works fine on local.

@andrewandante
Copy link
Contributor

Hi @mark-a-j-adriano - with the PHP8 upgrade this PR has gone quite stale I'm afraid 😣 do you want to take another swing at it, or rather convert it to an issue for someone else to tackle?

@satrun77 satrun77 force-pushed the feature/explicit-dataobject-for-subsite branch 2 times, most recently from 9d2a57a to 18af2bd Compare September 8, 2022 01:02
@mark-a-j-adriano mark-a-j-adriano changed the base branch from master to 2 September 8, 2022 06:08
@satrun77 satrun77 force-pushed the feature/explicit-dataobject-for-subsite branch from 18af2bd to 0f2e2fb Compare September 8, 2022 06:59
@mark-a-j-adriano
Copy link
Author

@andrewandante -> Looks like @satrun was able to add the test fix. Kindly check this again. @satrun Thanks for the assist.

@andrewandante
Copy link
Contributor

Hi team! If you need this for PHP7.X then you'll need to point it at the 1 branch, and if you want it to be for the PHP8.X branch (2) then please remove the travis.yml as testing has moved to Github actions :)

@satrun77
Copy link
Contributor

satrun77 commented Sep 8, 2022

Fix for version 2 and PHP8 only. I will remove travis :)

@satrun77
Copy link
Contributor

satrun77 commented Sep 9, 2022

I see that 🙂I will fix GitHub actions

@andrewandante
Copy link
Contributor

Some failures here - I think there is some linting to do, and also the subsites module needs to be included via composer perhaps?

src/Extensions/Subsites/IndexConfigurationExtension.php Outdated Show resolved Hide resolved
if (!isset($explicitClasses[$doc->getDataObject()->ClassName])) {
unset($indexes[$indexName]);

break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this mean that if it's in the second index, but not the first, it won't be indexed appropriately?

Copy link
Contributor

Choose a reason for hiding this comment

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

I will test this scenario and update PR.

src/Extensions/Subsites/IndexConfigurationExtension.php Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@satrun77 satrun77 force-pushed the feature/explicit-dataobject-for-subsite branch from 8128e4d to 9529cba Compare September 9, 2022 09:25
@satrun77 satrun77 force-pushed the feature/explicit-dataobject-for-subsite branch from 98a5286 to a8b918a Compare September 11, 2022 23:00
Copy link
Contributor

@andrewandante andrewandante left a comment

Choose a reason for hiding this comment

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

Code looks good to me, thanks for persisting! Please squash up your commits and then I'd be happy to merge 😄

@satrun77 satrun77 force-pushed the feature/explicit-dataobject-for-subsite branch from e42d05c to b945b6a Compare September 12, 2022 02:03
@andrewandante andrewandante merged commit 07b2866 into silverstripe:2 Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants