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

fixed missing own asset rule for parent recursive get access rule #17009

Merged
merged 6 commits into from Dec 5, 2019

Conversation

julienV
Copy link
Contributor

@julienV julienV commented Jul 6, 2017

Pull Request for Issue # .

Summary of Changes

Fixed a failing strict comparison operator, which prevents parent recursive asset rule to work properly
self::$assetPermissionsParentIdMapping[$extensionName][$id] is the result of a database query, so the type of self::$assetPermissionsParentIdMapping[$extensionName][$id]->id is string.
$assetId if an integer, so the test result is false, but it should be true...

selection_168

Testing Instructions

Not easy to test, as this parameters don't seem to be use directly in joomla core...

Expected result

Actual result

Documentation Changes Required

none

@csthomas
Copy link
Contributor

csthomas commented Jul 7, 2017

IMO It would be better to set this more explicitly, I mean using (int) $a !== $b. I know that does not change anything in php but for people it is more explicit.

@julienV
Copy link
Contributor Author

julienV commented Jul 7, 2017

sure, i can change it to
(int) self::$assetPermissionsParentIdMapping[$extensionName][$id]->id, but does it really makes it more readable ? the variable name is so long, you don't really see the type casting.

@ggppdk
Copy link
Contributor

ggppdk commented Jul 7, 2017

sure, i can change it to
(int) self::$assetPermissionsParentIdMapping[$extensionName][$id]->id, but does it really makes it more readable ? the variable name is so long, you don't really see the type casting.

it a not only about readability, which i argue it is quite readable , it is not too long,
it will also make clear to people looking at the code in the future what the comparison does, thus avoid the bug being re-introduced by future modifications

@julienV
Copy link
Contributor Author

julienV commented Jul 24, 2017

@ggppdk i added the casting to int

@mbabker
Copy link
Contributor

mbabker commented Jul 21, 2018

All this critique about type casting and no follow on...

(Needs merge conflicts resolved)

@julienV
Copy link
Contributor Author

julienV commented Jul 21, 2018

there was no conflict, merging of staging branch went fine...

@ghost ghost added the J3 Issue label Apr 5, 2019
@ghost ghost removed the J3 Issue label Apr 19, 2019
@ghost ghost changed the title [fix] fixed missing own asset rule for parent recursive get access rule fixed missing own asset rule for parent recursive get access rule Apr 19, 2019
@SharkyKZ
Copy link
Contributor

I have tested this item ✅ successfully on fc4d255


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17009.

@Quy
Copy link
Contributor

Quy commented Nov 8, 2019

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17009.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 8, 2019
@HLeithner
Copy link
Member

Finally a happy end. Thanks

@HLeithner HLeithner merged commit 6b97b65 into joomla:staging Dec 5, 2019
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 5, 2019
@HLeithner HLeithner added this to the Joomla! 3.9.14 milestone Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants