Views panes allows many of its settings to be configured once globally and another time on the pane configuration. For each of these settings the administrator can control individually whether it is allowed to override them.

However, the logic in panels_views_render(), which determines whether to use the global or the overridden value seems to be broken:

if (($pv->allow_more_link && !empty($conf['more_link'])) ||
  (!$pv->allow_more_link && $pv->more_link)) {
  ...

1. If overriding is allowed, and there is an overidden value, then use it - ok.
2. However, the globally configured value is only ever used, if the checkbox that controls whether overriding is allowed, is unchecked. This means that when overriding is allowed, but the override value is empty, the global value will not be taken into account.

The fixed logic looks like this:

if (($pv->allow_more_link && !empty($conf['more_link'])) || $pv->more_link) {
CommentFileSizeAuthor
#5 panels-views-settings.patch3.8 KBsmk-ka
panels_views-logic.patch2.06 KBsmk-ka

Comments

merlinofchaos’s picture

merlinofchaos commented
Status: Needs review » Needs work

I am not sure I agree with this. With your logic, try this:

Set the 'more link' to be allowed, and set the default to checked. Then go to the pane. Leave more link unchecked. I bet you will get a more link when the user expects not to.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I have tested this and can confirm that Earl's assumption is right.

sdboyer’s picture

sdboyer commented

Given that all of the changes in the patch exhibit the same problem merlin highlighted in #1, I don't know that CNR is accurate. Is there actually something to salvage in this patch, or should we just wontfix it?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

If I get this right,

  1. $pv->allow_more_link && !empty($conf['more_link']) displays the link if overriding is allowed and the pane configuration option is enabled.
  2. !$pv->allow_more_link && $pv->more_link displays the link if overriding is not allowed, but the link is enabled in view pane configuration.
  3. Neither of both conditions checks, whether overriding is allowed, the link is enabled in view pane configuration, but has not been set in the pane configuration yet - that is, what happens when you set up a view pane first, add it to one or more displays, and then find out that you need to allow overriding the option in the pane configuration for one of those panes. Hence, only 1 of more panes has a configured $conf, but you need to re-visit and re-save all pane configurations to get the same result like before.
smk-ka’s picture

smk-ka commented
Status: Needs work » Needs review
StatusFileSize
new panels-views-settings.patch3.8 KB

Created a new patch which adresses the issue Earl and Daniel described. The two crucial changes are:
1. The override condition has been factored out into a new function (and looks pretty harmless there).
2. To prevent E_ALL notices from popping up, the $conf array needs to be sanitized before accessing possibly unset keys. In turn, this eases accessing its values later.

The new settings function returns the locally configured setting only if it has been set (ie. it has a non-NULL value), otherwise it returns the global setting. The remaining if-conditions that can't make use of the new function have been changed to follow the new logic.

Two questions arised during studying the code (unrelated to this specific issue):
A. The 'Set view URL to panel URL' setting (look for $conf['url_from_panel']) seems to be not used throughout the whole module (ie. you can configure it but it is nowhere applied). Is it gone, to-do, or am I blind?
B. Why is a manually entered pager_id incremented by one? Shouldn't this be just intval($conf['pager_id'])?
$pager_id = $conf['use_pager'] ? (intval($conf['pager_id']) + 1) : 0;

merlinofchaos’s picture

merlinofchaos commented

smk-ka: A) sounds like an oversight and might be why people keep posting bug reports about that feature. If that is not used it is definitely incorrect behavior. B) is because Views uses a very silly notion for pager IDs in Views 1 and it overloads the 'use_pager' setting. If use_pager > 0, pager_id == use_pager - 1. I realized later this was a bad idea but that's the situation there.

This new patch needs to still be reviewed.

sdboyer’s picture

sdboyer commented
Status: Needs review » Needs work

&^#@!*$^!@^&$!@ at d.o

it ate my damn post. ffs, I had all these detailed responses written up, too.

Summary: this patch still needs work. It actually seems to do the reverse of what's claimed in the OP, whereas the original code DOES do it properly.

esmerel’s picture

esmerel commented
Status: Needs work » Closed (won't fix)

No changes to be made to 2.x code