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

FIX only exclude SiteTree subclasses if there are any set. #60

Conversation

fspringveldt
Copy link

LumberJack::liveChildren() and LumberJack::stageChildren() should check if count(LumberJack::getExcludedSiteTreeClassNames()) > 0 to avoid an InvalidArgumentException exception being thrown by DataList::exclude().

@robbieaverill
Copy link

I think we should make this PR for the SS3 compatible version and merge up.

Have requested a review from @micmania1 for the logic

@robbieaverill
Copy link

We need to update the travis config - logged as #61

@fspringveldt fspringveldt changed the base branch from master to 1.3 September 19, 2017 03:37
@fspringveldt fspringveldt force-pushed the pulls/2/fix-zero-count-excludedsitetreeclassnames branch from f88270b to a31bdc2 Compare September 19, 2017 04:15
@fspringveldt fspringveldt force-pushed the pulls/2/fix-zero-count-excludedsitetreeclassnames branch from a31bdc2 to dd99980 Compare September 19, 2017 04:25
@fspringveldt
Copy link
Author

Okay, rebased off 1.3 branch instead.

@micmania1
Copy link

I think the check can just be added to the shouldFilter() function.

The reason being; it's a bit misleading when shouldFilter() returns true because that implies that we should filter, but then we don't because count() returns 0.

@fspringveldt
Copy link
Author

fspringveldt commented Sep 19, 2017

Thanks @micmania1

I think the check can just be added to the shouldFilter() function.

I considered this (after @robbieaverill suggested it too), but decided against.

The single responsibility of shouldFilter() is "Checks if we're on a controller where we should filter".

This additional check (::excludeSiteTreeClassNames()) mutates a DataList based on whether we shouldFilter() and if there any configured hidden subclasses (result of ::getExcludedSiteTreeClassNames()).

Thus, implementing this on LumberJack::shouldFilter() would mean updating everywhere it's used (e.g. BlogFilter::stageChildren()), which doesn't necessarily care about hidden subclasses.

Thoughts?

@micmania1
Copy link

I can see how it would affect blog. In that case could you just make the new method excludeSiteTreeClassNames protected since its only use is internal? If we need to make it public later we can do that easily without introducing breaking changes.

Could you also add further notes to the shouldFilter docblocks so that its absolutely clear that it only checks the current controller and maybe link back to this discussion?

Copy link

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

We can't do this in 1.3 any more, will need to go back to targetting master

@robbieaverill robbieaverill dismissed their stale review September 21, 2017 01:30

I only read one commit, sorry

@micmania1 micmania1 merged commit 87af7ee into silverstripe:1.3 Sep 21, 2017
@fspringveldt fspringveldt deleted the pulls/2/fix-zero-count-excludedsitetreeclassnames branch September 21, 2017 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants