Bug #50873
closedsectionIndex menu is empty when the page is not in menu
Added by Rémy DANIEL about 11 years ago. Updated almost 7 years ago.
100%
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)
Files
patch_AbstractMenuContentObject.php (558 Bytes) patch_AbstractMenuContentObject.php | Rémy DANIEL, 2013-08-07 14:34 | ||
patch2_AbstractMenuContentObject.php (788 Bytes) patch2_AbstractMenuContentObject.php | Alexander von Drach, 2014-05-20 00:47 | ||
issue_50873.patch (2.74 KB) issue_50873.patch | an approach using new TS settings | Alexander von Drach, 2014-05-20 13:55 |
Updated by Marc Bastian Heinrichs almost 11 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
Updated by Gerrit Code Review almost 11 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
Updated by Rémy DANIEL almost 11 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
Updated by Marc Bastian Heinrichs almost 11 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.
Updated by Rémy DANIEL almost 11 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...
Updated by Alexander von Drach about 10 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)
Updated by Marc Bastian Heinrichs about 10 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 setnav_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.
Updated by Alexander von Drach about 10 years ago
- File issue_50873.patch issue_50873.patch added
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.
- 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 becomeincludeNotInMenu = 1
sectionIndex {
includeHiddenHeaders = 1
includeEmptyHeaders = 1
}
sectionIndex.type = header
would becomeincludeNotInMenu = 0
sectionIndex {
includeHiddenHeaders = 0
includeEmptyHeaders = 0
}
(which would be default)
And sectionIndex.type = anything_else
would becomeincludeNotInMenu = 0
sectionIndex {
includeHiddenHeaders = 1
includeEmptyHeaders = 1
}
And includeNotInMenu = x
would becomesectionIndex.includeHiddenPages = x
The attached patch implements this (and removes the obsolete TS).
This would of course require a documentation change as well.
Updated by Marc Bastian Heinrichs about 10 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.
Updated by Alexander von Drach about 10 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.
Updated by Gerrit Code Review over 9 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
Updated by Gerrit Code Review over 9 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
Updated by Susanne Moog over 9 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
Updated by Rémy DANIEL over 9 years ago
Susanne, this was exactly the purpose of my first patch :)
Updated by Benni Mack over 9 years ago
- Target version changed from 6.2.0 to 7.1 (Cleanup)
- Sprint Focus set to On Location Sprint
Updated by Gerrit Code Review over 9 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
Updated by Benni Mack over 9 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset a1707ef5ab222ea457e66b45ad7f7484cccb70d8.
Updated by Anja Leichsenring over 8 years ago
- Sprint Focus deleted (
On Location Sprint)
Updated by Riccardo De Contardi almost 7 years ago
- Status changed from Resolved to Closed