Bug #50873

sectionIndex menu is empty when the page is not in menu

Added by DANIEL Rémy over 6 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Content Rendering
Target version:
Start date:
2013-08-07
Due date:
% Done:

100%

TYPO3 Version:
6.1
PHP Version:
5.4
Tags:
Complexity:
Is Regression:
No
Sprint Focus:

Description

In AbstractMenuContentObject::sectionIndex(), sectionIndex menu items are first based on the page record, and then overridden with tt_content properties.

But if $basePageRow has the 'nav_hide' property set to 1, then the resulting menuItem will systematically failed in AbstractMenuContentObject::filterMenuPages().

We have already the tt_content's 'sectionIndex' property in order to exclude a tt_content element from a sectionIndex menu, so I see no reason to not to unset 'nav_hide'.

To fix that, we need to force 'nav_hide' on the menuItem to 0 in AbstractMenuContentObject::sectionIndex().

Patch included, tested on TYPO3 6.1 (should be OK on earlier version)

patch_AbstractMenuContentObject.php View (558 Bytes) DANIEL Rémy, 2013-08-07 14:34

patch2_AbstractMenuContentObject.php View (788 Bytes) Alexander von Drach, 2014-05-20 00:47

issue_50873.patch View - an approach using new TS settings (2.74 KB) Alexander von Drach, 2014-05-20 13:55

Associated revisions

Revision a1707ef5 (diff)
Added by Benni Mack almost 5 years ago

[BUGFIX] Render section index menu if page is not in menu

This patch fixes the behaviour that a section index menu
is not rendered, if the page whose sections should be shown
has the flag "no in menu".

Resolves: #50873
Releases: master
Change-Id: Ibe1c0ac99d225ab4f9d315d576c39f477e03a3b7
Reviewed-on: http://review.typo3.org/24872
Reviewed-by: DANIEL Rémy <>
Tested-by: DANIEL Rémy <>
Reviewed-by: Susanne Moog <>
Tested-by: Susanne Moog <>
Reviewed-by: Benjamin Mack <>
Tested-by: Benjamin Mack <>

History

#1 Updated by Marc Bastian Heinrichs over 6 years ago

  • Status changed from New to Accepted
  • Target version changed from next-patchlevel to 6.2.0
  • Is Regression set to No

bug confirmed for current master, 6.1, and 6.0

#2 Updated by Gerrit Code Review over 6 years ago

  • Status changed from Accepted to Under Review

Patch set 1 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/24872

#3 Updated by DANIEL Rémy over 6 years ago

Your solution seams a little hacky to me.
You change the default rendering of sectionIndex menus in css_styled_content, but this does not address sectionIndex menu built from scratch.

When I build a HMENU with sectionIndex, I expect that this menu renders consistently if the page where it is added is "in menu" or "not in menu". The sectionIndex menu is, for me, just an element of the page (like any other tt_content), and it should be rendered on the page regardless of the page status.

My proposal address the described behavior : the sectionIndex menu will be rendered on the page, regardless of the status of the page's property on which it is inserted, and without the need of a TypoScript hack.

If your solution is still accepted by the majority, then could you fix also the documentation of HMENU.sectionIndex, to point that "needed" TypoScript hack ?

Best regards
Rémy

#4 Updated by Marc Bastian Heinrichs over 6 years ago

Hey Daniel,

thx for your feedback. To me the whole section index php code seems hacky... building "virtual" page records from tt_content rows... not the best way IMO.

So I used the typoscript setting which is made for such a usecase.
But you are abolutely right, that this needs to be documented then.

#5 Updated by DANIEL Rémy over 6 years ago

I still think that the sectionIndex menu should be rendered on the page, regardless of the status of the page's "nav_hide" property on which it is inserted, and without the need of a TypoScript hack...

#6 Updated by Alexander von Drach almost 6 years ago

I also have to deal with this problem, but I think the above patch is not the best solution, it's simply a hard-coded includeNotInMenu=1 for section index menues. Nor is https://review.typo3.org/24872, because wwhile you can still change the setting for menues, this won't allow you to have a section index menu on a page which is hidden in menues, and also hide some content elements from the section index.

A better solution would be to respect the sectionIndex flag in the content record,

So instead of
$result[$uid]['nav_hide'] = '0';
it should be
$result[$uid]['nav_hide'] = !$row['sectionIndex'];

Or even better, add AND sectionIndex=1 to the query fetching the content elements (and set nav_hide to 0), see the applied patch.

(Edit: this is wrong, see below)

#7 Updated by Marc Bastian Heinrichs almost 6 years ago

Alexander von Drach wrote:

I also have to deal with this problem, but I think the above patch is not the best solution, it's simply a hard-coded includeNotInMenu=1 for section index menues. Nor is https://review.typo3.org/24872, because wwhile you can still change the setting for menues, this won't allow you to have a section index menu on a page which is hidden in menues, and also hide some content elements from the section index.

A better solution would be to respect the sectionIndex flag in the content record,

So instead of
$result[$uid]['nav_hide'] = '0';
it should be
$result[$uid]['nav_hide'] = !$row['sectionIndex'];

Or even better, add AND sectionIndex=1 to the query fetching the content elements (and set nav_hide to 0), see the applied patch.

The nav_hide thing and hiding elements from section index are two separate things and the last works correct. Check for

$doIncludeInSectionIndex = $row['sectionIndex'] >= 1;

in the code.

Adding this in Typoscript is no "hack". It uses a simple setting which is made for such a usecase.
Hardcoding this no one could simple change the behaviour. Using settings, that are already there is the best IMO. Hacking this virtual page records created from content records in addition is more a bad practice than using a simple setting.
But you are invited to push another changeset for this issue and if it will get enough votes, fine with it.

#8 Updated by Alexander von Drach almost 6 years ago

Please ignore my above patch, it is incorrect since it assumes that tt_content.sectionIndex can't be overwritten in translated elements.

I think this whole issue needs some rework.

I don't get why pages and content elements are treated differently regarding the TS configuration (includeNotInMenu vs sectionIndex.type).
Also, the proper usage of sectionIndex.type is unclear, and does not allow all possibilities. For example, it is not possible to include empty headers, but not hidden one, or include those with sectionIndex = 0, but exclude invalid headers.

Imho
  • includeNotInMenu should always refer to the elements displayed in the menu, which are content elements in case of a section index (even if the code uses modified page records).
  • This means nav_hide of the menu entry should be set according to the value of sectionIndex, so that includeNotInMenu will handle this as expected.
  • Since it is possible to create section indexes that span mutliple pages, it might be useful to allow exclusion of pages that are hidden in menues, but a different option should be used for this, like sectionIndex.includeHiddenPages.
  • sectionIndex.type should be removed, instead use sectionIndex.includeEmptyHeaders, the already existing sectionIndex.includeHiddenHeaders and sectionIndex.includeHiddenPages.

So
sectionIndex.type = all
would become
includeNotInMenu = 1
sectionIndex {
includeHiddenHeaders = 1
includeEmptyHeaders = 1
}

sectionIndex.type = header
would become
includeNotInMenu = 0
sectionIndex {
includeHiddenHeaders = 0
includeEmptyHeaders = 0
}

(which would be default)

And
sectionIndex.type = anything_else
would become
includeNotInMenu = 0
sectionIndex {
includeHiddenHeaders = 1
includeEmptyHeaders = 1
}

And
includeNotInMenu = x
would become
sectionIndex.includeHiddenPages = x

The attached patch implements this (and removes the obsolete TS).
This would of course require a documentation change as well.

#9 Updated by Marc Bastian Heinrichs almost 6 years ago

Alexander von Drach wrote:

Please ignore my above patch, it is incorrect since it assumes that tt_content.sectionIndex can't be overwritten in translated elements.

Ok

I think this whole issue needs some rework.

The first impression is, that this is more than a bugfix, that could go into the current version.

I don't get why pages and content elements are treated differently regarding the TS configuration (includeNotInMenu vs sectionIndex.type).

Because basically a HMENU renders pages. But here it is used (since 4.6) to render a menu of content elements. Till 4.5 a CONTENT object was used for the sectionIndex ( https://git.typo3.org/Packages/TYPO3.CMS.git/blob/master:/typo3/sysext/css_styled_content/static/v4.5/setup.txt#l1455 )
And the basic problem is, that the page properties are copied and overwritten with the content data. https://git.typo3.org/Packages/TYPO3.CMS.git/blob/master:/typo3/sysext/frontend/Classes/ContentObject/Menu/AbstractMenuContentObject.php#l1866

This is why you could have settings regarding the page (includeNotInMenu) and the content (sectionIndex.type) properties here.

#10 Updated by Alexander von Drach almost 6 years ago

Marc Bastian Heinrichs wrote:

The nav_hide thing and hiding elements from section index are two separate things and the last works correct. Check for
[...]
in the code.

Yes, indeed. I got that wrong yesterday.

Adding this in Typoscript is no "hack". It uses a simple setting which is made for such a usecase.

Here is disagree. Well, it's no hack - however, he setting is not simple, but quite confusing and not intuitive.
Also it is limiting your options (see my previous comment).

Hardcoding this no one could simple change the behaviour. Using settings, that are already there is the best IMO. Hacking this virtual page records created from content records in addition is more a bad practice than using a simple setting.

Hardcoding is bad indeed (and I also missed the point that there might be a section index spawning multiple pages, where some might be hidden in menues, so ignoring nav_hide of the page is also incorrect).

Using settings is the correct way, however, if existing settings are confusing and preventing possibilities, they should be changed.

My main point is that includeNotInMenu should refer to the entities actually displayed in the menu. This is imho more logical than always referring it to the page (especially when almost all section indexes will only contain elements from the same page that contains them, where it is obvious that all elements should be included).

The first impression is, that this is more than a bugfix, that could go into the current version.

Yes, it is more, and possible side effects (i.e. breaking the configuration of existing intallations) should be carefully considered before adding this to released versions. And your patch should indeed be sufficient for most cases (if not all existing, since the impossible cases are rather exotic).
But I would like to see it changed in coming versions.

#11 Updated by Gerrit Code Review over 5 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/24872

#12 Updated by Gerrit Code Review over 5 years ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/24872

#13 Updated by Susanne Moog over 5 years ago

Hi,

maybe I'm missing something, but wouldn't unsetting nav_hide function sectionIndex in the AbstractMenuContentObject work?

That method is only used to render sections. So the case(s) would be:

Page has set nav_hide, has sections which can be configured to be displayed by setting the sectionIndex checkbox.
Page has not set nav_hide, sections same as above.

In both cases I would expect a section menu to show all sections configured to be shown, independent from the page they are on. Unsetting $result[$uid]['nav_hide'] in said method should do that (as the skipping of records with sectionIndex disabled is done separately).

Is there another case to consider?

Thanks,

Susi

#14 Updated by DANIEL Rémy over 5 years ago

Susanne, this was exactly the purpose of my first patch :)

#15 Updated by Benni Mack about 5 years ago

  • Target version changed from 6.2.0 to 7.1 (Cleanup)
  • Sprint Focus set to On Location Sprint

#16 Updated by Gerrit Code Review almost 5 years ago

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/24872

#17 Updated by Benni Mack almost 5 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

#18 Updated by Anja Leichsenring about 4 years ago

  • Sprint Focus deleted (On Location Sprint)

#19 Updated by Riccardo De Contardi over 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF