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

Correctly handle root nodes for the dcaPicker #1001

Closed
wants to merge 2 commits into from

Conversation

agoat
Copy link
Contributor

@agoat agoat commented Aug 4, 2017

The handling of the rootNodes in the new dcaPicker is not working right for non admin backend users.
If the backend user is restricted to a page mounts and also a rootNode from a different level is defined as attribute for the picker, no pages will be shown at all.

Little example:

Let's say we have the following page structure
rootnodes

If the user is restricted to ID 5 and a rootNode ID 1 is set as an attribute for the picker no pages will be shown. That's because the ID 1 is not a child of ID 5.
The same happens when the attribute rootNode for the picker is also ID 5. Because the rootNode itself is not included for the array intersection.

So first the given rootNode(s) have to be added for the intersection. And secondly all the children have to be traversed down for the the rootNode attribute, too.

@aschempp
Copy link
Member

aschempp commented Aug 4, 2017

Interesting.

Why do we use $GLOBALS['TL_DCA'][$this->strTable]['list']['sorting']['root'] instead of $this->root in the first place??

@agoat
Copy link
Contributor Author

agoat commented Aug 4, 2017

Why do we use $GLOBALS['TL_DCA'][$this->strTable]['list']['sorting']['root'] instead of $this->root in the first place??

Not sure what you want to know exactly.

But it looks like that in $this->root the current user page mount is already stored and we don't need to take it again from $GLOBALS['TL_DCA'][$this->strTable]['list']['sorting']['root'].

However, the basic problem remains. If the user page mount is on a childpage and the roodNode attribute is set to the parent page, no pages will be shown. We have to use getChildRecords on both - rootNodes and $this->root.

@leofeyer
Copy link
Member

leofeyer commented Aug 9, 2017

How exactly can I reproduce this?

@agoat
Copy link
Contributor Author

agoat commented Aug 9, 2017

How exactly can I reproduce this?

Let's say we have the following page structure:
pages2

Create a user and set the pagemount to 'subfolder'.
Now change (for testing purpose) the url field in the tl_content.php to the following:

		'url' => array
		(
			'label'                   => &$GLOBALS['TL_LANG']['MSC']['url'],
			'exclude'                 => true,
			'search'                  => true,
			'inputType'               => 'text',
			'eval'                    => array
                        (
				'mandatory'		=> true, 
				'rgxp'			=> 'url', 
				'decodeEntities'	=> true, 
				'maxlength'		=> 255, 
				'dcaPicker'		=> true,
				'fieldType'		=> 'radio', 
				'filesOnly'		=> true, 
				'rootNodes'		=> array(ID of 'test root' or 'subfolder')
				'tl_class'		=>'w50 wizard'
			),
			'sql'                     => "varchar(255) NOT NULL default ''"
		),

If you now try to select a page in the hyperlink ce, no pages will be shown.

@leofeyer leofeyer added this to the 4.4.3 milestone Aug 15, 2017
@leofeyer leofeyer self-assigned this Aug 15, 2017
@leofeyer
Copy link
Member

leofeyer commented Aug 15, 2017

Ok, these are actually two cases:

Page mount ID5 + root nodes ID5

This is clearly a bug, which I have fixed in c0ca437.

Page mount ID5 + root nodes ID1

This does not seem like a bug to me, because if a user is not allowed to see anything above ID5 and the admin configures ID1, there is no intersection – not even if some of the user's page mounts actually match some subpages. But we can sure discuss this case with the other @contao/developers.

@agoat
Copy link
Contributor Author

agoat commented Aug 16, 2017

As an example: I have an extension which restrict the selection to the current page root (think about multi domain or multi language setups).

So I will pass the root nodes ID1 and the user can select what is the result of the intersection of all child pages. Otherwise I first need to get the page mount of the user to do it myself but afterwards the half check is done again.

IMO it makes totally sence to support root node IDs above or outside of the page mount children range.

@leofeyer leofeyer modified the milestone: 4.4.3 Aug 16, 2017
@leofeyer
Copy link
Member

leofeyer commented Sep 7, 2017

As discussed in Mumble on September 7th, we want to support your use case. However, we are not sure about the array_merge() solution. Did you test this with multiple root nodes (e.g. $arrRoot = array(1, 5)?

@leofeyer leofeyer added this to the 4.4.5 milestone Sep 7, 2017
@agoat
Copy link
Contributor Author

agoat commented Sep 14, 2017

I tested this with multiple root nodes from root pages by setting the ids of all multilingual root pages. No problems here.

But when I set (why ever) a root and a child page id, like your example $arrRoot = array(1, 5), it returned the resulting root page(s) multiple times. So we have to be sure to return only unique values.

Fixed in 409e08c.

$arrRoot = array_intersect($arrRoot, array_merge($this->root, $this->Database->getChildRecords($this->root, $this->strTable)));
$arrRoot = $this->eliminateNestedPages($arrRoot);
$arrRoot = $this->eliminateNestedPages(array_unique($arrRoot, SORT_NUMERIC));
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add the SORT_NUMERIC flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why did you add the SORT_NUMERIC flag?

To compare the array values numerically.

But your solution in f3aafc0 works, too.

leofeyer added a commit that referenced this pull request Sep 15, 2017
@leofeyer
Copy link
Member

Fixed in f3aafc0.

@leofeyer leofeyer closed this Sep 15, 2017
@agoat agoat deleted the fix/intersectRootNodes branch September 18, 2017 07:26
@leofeyer leofeyer modified the milestones: 4.4.5, 4.4 May 14, 2019
leofeyer added a commit that referenced this pull request Nov 21, 2019
Description
-----------

Fixes #793

Commits
-------

94518d99 Hide the "cols" field in the login module settings (see #793)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants