Fix #EZP-19887 : ignore_visility parameter should always be false by default

[ballinette]

trafficstars

Fix "ignore_visility" behaviour in content/search, content/related_objects, content/related_objects_count, content/reverse_related_objects and content/reverse_related_objects_count :

Parameter's value should always be "false" by default (and not null) Consequences : fix admin templates to force "ignore_visilibty=true" when necessary Fix #EZP-19887

[gggeek]

+1. But needs a paragraph in BC notes

[nfrp]

Thanks Gilles!

Looks ok to me. I added a little inline note on a potentially useless check.

@gggeek: do you see any BC here?

[gggeek]

The sig of fetchRelatedObjectsCount and fetchRelatedObjects seems to be different (and their behaviour too)

[nfrp]

Yep, signatures have changed, but the behavior is identical in the end, it seems. The !== null check in both methods turns out to be void, unless a null value is intentionally passed (would be strange), and the default value is safe: no ignoring visibility.

+1 on my end, after a little note has been added, as you rightly noted, in the BC docs concerning the change in signature.

@ballinette : do you think you could add this note?

Thanks!

[ballinette]

OK guys. I have also removed a useless line in fetchRelatedObjects method.

Can you tell me if my BC note is OK ?

[achasseux]

Hello,

:+1: This is a good improvement, but I think there is a bug that you can fix at the same time. In the method eZContentObject::relatedObjectCount(), the definition of $showInvisibleNodesCond is the following :

$showInvisibleNodesCond = self::createFilterByVisibilitySQLString( $params['IgnoreVisibility'], 'inner_object' );

And it can be advantageously changed to the following :

$showInvisibleNodesCond = self::createFilterByVisibilitySQLString( $params['IgnoreVisibility'],
                                                                   $reverseRelatedObjects ? 'inner_object' : 'outer_object' );

Since inner_objet represents the source while outer_objet represents the target of the relationship, in the case of direct relationship, we want to check the visibility of the target, but not of the source ! On the other side, in the case of reverse relationships, outer_object and inner_object both represent the source object of the SQL query, and they could be interchanged on the condition $showInvisibleNodesCond without changing the result. (the SQL query is not symmetric)

Are you agree with me ? I'm wrong ?

Arnaud.

[ballinette]

Hi @achasseux : I think the bug you're talking about is another subject, and it could be treated into another PR, no ?

@gggeek , @nfrp : what do you think ? And is my fix OK to be merged now or should I fix something else ?

Thx

[achasseux]

Hi @ballinette, yes I'm talking about another subject. But I didn't want to fork at the same time. I will fork when I will have a moment. Thanks.

[andrerom]

The only issue with this is that null is most likely sent by intention, null is supposed to be treated as using siteaccess settings for visibility at a deeper level, so if it is not, then we have a bug. At least that is how subtree fetch works afaik.

[ballinette]

Well, I think you're right, @andrerom ;) we should better keep default value for ignore_visibility to null, and in this case, use the ShowHiddenNode settings to set the correct value... Is it what you meant ?

I can try to update my PR to fix that ;)

[andrerom]

Is it what you meant ?

Exactly what I meant :)

[ballinette]

Hi @andrerom I changed the behaviour according to what we said.

Now, all content fetch functions (content/list(_count), content/tree(count), content/search, content/(reverse)related_objects(_count) use NULL ignore_visibility parameter by default, and in this case, they use settings site.ini/SiteAccessSettings/ShowHiddenNodes to set its value.

Is it OK for you ?

[andrerom]

It is ok as far as I can see. Besides the small comment above, there are some micro CS issues (missing empty new line after the new if blocks), and BC note should have text describing the change in behavior of all the affected fetch functions.

like

- Fixed EZP-19887: Always use settings when ignore_visibility is undefined (or set to null)
    - fetch functions content/list, content/list_count, content/tree, content/tree_count all now
       has ignore_visibility set to null by default to use ini setting instead of previous "false"
    - Fetch function content/related_objects_count now supports ignore_visibility like the others

Could you fix those and also rebase on master and push to get travis to run tests?

Side note: While on this I found use of ignore_visibility in ezwebin and ezdemo related to calendar views which is probably wrong.

[ballinette]

Hi @andrerom I've rebased the code and completed the BC notes ;)

I'll take a look look on ezwebin and ezdemo also..

[andrerom]

Hi, knew I had forgot to update this. We had feature freeze on 5.2 a few weeks ago and have branched in the mean time, so sorry but I'm afraid you need to do one small change of changing 5.2 to 5.3 on this :/ But ok, we can do that while merging it also.

For the PR: +1 /cc @lolautruche

[gggeek]

+1

[yannickroger]

+1