A few improvements for assets #14319

csthomas · 2017-03-03T15:07:02Z

Pull Request for a few fixes from my previous non B/C PR #14279

Summary of Changes

Do not force to set rules from com_content component to every new article.
Use inheritance (simple '{}') as in other extensions.
Clean empty actions from rules: {'core.edit': {}, 'core.delete: {}'} to {}
Use faster JAccessRules::getData() instead of json_decode() and json_encode() in method JAccessRules::__toString()
Add missing column asset_id in tests/unit/schema/ddl.sql

Testing Instructions

Install joomla (I have installed with sample - Test English)
Go to backend Content -> Articles -> click on button Options -> select Tab Permissions
Change permissions for Manager from 'Inherit' to 'Allowed' for Create, Edit, Delete:
Save and Close.
Create a new Article, in permission tab as above(for Manager) permission are set as 'Inherit' - it is OK.
Save and check again - Now a few permissions has been changed from 'Inherit' to 'Allowed' - it is not OK.
Apply patch
Back to point 5 (create a new article)
After save, permissions for Manager stay 'Inherit'. - that is now OK.
Change permission for Manager for Edit action from 'Inherit' to 'Allowed' and Save.
Change tab to user group 'Editor':
Action Edit is as 'Allowed'. Change it to 'Inherit' and Save.
Back to permissions for Manager and check if Edit action has not been reset and still has selected 'Allowed'.

Expected result

A new article by default inherit permissions from parent category (after stored).

Actual result

A new article after save always has copy of permissions from com_content asset.

Documentation Changes Required

None

… up inherited permissions

ghost · 2017-03-05T08:35:59Z

Questions about Test Instructions:

Go to backend and create a new article, go to permissions tab and check if permissions are sets as 'inherit'.

Check in each User Group for each Action?

After save it should stay as 'inherit'.

same as above.

If you set 'inherit' for group ex. Manager then it should not reset permissions for other groups.

sounds like this is the Problem but on testing i see no "reset permissions for other groups"

I don't understand the Issue and what to test.

csthomas · 2017-03-05T11:52:11Z

@franz-wohlkoenig Some time ago I had a bug which reset permission to 'inherit' for other user groups if I only changed permission to 'inherit' for only one user group.

[Updated] Please take a look at the new test instruction.

ghost · 2017-03-05T12:32:27Z

@csthomas will test today, thanks for Infos.

ghost · 2017-03-05T13:23:06Z

Using default user groups:

and work your Example:

In article edit, permission tab, set permission for Edit to "Allowed" for Manager and Publisher.
After save article, change permission for Edit to "Inherit" for only Manager and save.
Now check if permission for Publisher is still Allowed and not 'Inherit'.

with and -out PR got in Manager: Inherited and in Publisher: Allowed (different Trees).

Creating an Article (before Save) all Groups are inherited.

csthomas · 2017-03-06T11:28:15Z

@franz-wohlkoenig I have made a new better test instruction. Please test again.

ghost · 2017-03-06T12:20:19Z

thanks @csthomas will test later.

ghost · 2017-03-06T13:30:55Z

Action Edit is as 'Allowed'. Change it to 'Inherit' and Save.

After Save Action Edit is set on Inherited. A Difference to Instructions the sad it is Allowed.

csthomas · 2017-03-06T13:33:38Z

Allowed should be preserved/not changed on different user group permission 'Manager'.

csthomas · 2017-03-06T13:41:55Z

Manager and Editor has the same value 'Allowed' for Edit action field.
Now you change for Editor this field to 'Inherit' and check if only this field has been changed and second one still is as 'Allowed' (I mean for Manager)

ghost · 2017-03-06T14:43:47Z

Group Editor has Setting Inherited, not Allowed like wrote in Instructions (thanks for clear Instructions).

csthomas · 2017-03-06T15:18:28Z

I suppose you do not understand me, May be I explained it in a wrong way:
Please test again points 10..13:

on patched joomla, on some existed article, only Edit action is important.
for Editor and Manager user group

On start, these two user groups has to be set as "Allowed".
As we have "Allowed" saved we can go to next step.

Change permission to "Inherit" for only one user group and save.
After that one (changed) user group should be as 'Inherit' and second should stay as 'Allowed'.

ghost · 2017-03-06T15:38:32Z

Looks like misunderstanding, i try in another Words:

Change tab to user group 'Editor': Action Edit is as 'Allowed'. Change it to 'Inherit' and Save.

I understand that opening Usergroup Editor Action Edit ist set on Allowed, but its set on Inherited:

Action Edit is as 'Allowed'. Change it to 'Inherit' and Save.

So this is not possible cause Inherited set.

csthomas · 2017-03-06T15:46:40Z

I understand that opening Usergroup Editor Action Edit ist set on Allowed, but its set on Inherited

But before you have to toggle to Manager tab to check that.

You have to do changes in Editor, but check Manager.
When you save setting for Editor group with 'Inherit' value then php code do some clean up for array, and should not erase too much. I means should not change settings for other user group like Manager.

ghost · 2017-03-06T15:55:33Z

I have tested this item ✅ successfully on 08d10b2

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14319.}

csthomas · 2017-03-06T15:58:22Z

Thanks for your patience :)

ghost · 2017-03-06T16:00:25Z

Thanks for your patience too ;-)

sanderpotjer · 2017-04-30T21:13:13Z

I have tested this item ✅ successfully on 08d10b2

Great PR, thanks!

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14319.}

ghost · 2017-05-01T06:52:17Z

RTC after two successful tests.

olleharstedt · 2017-07-06T10:16:58Z

After updating to 3.7.3, we got "Invalid parent ID" for our own JTable. Is this commit related?

rdeutz · 2017-07-06T10:41:06Z

@olleharstedt please open a new issue, comments on closed/merged PR get lost

csthomas · 2017-07-06T10:41:57Z

Please open a new issue, but meantime:

You may try to revert libraries/joomla/table/asset.php for test or other files too to version from 3.7.2, but the reason is that in your db table #__assets does not exist parent row.

Check your table and find which assets rows has invalid parent_id, means parent_id points to not existed row id.

Example query:

SELECT a.* FROM `#__assets` a LEFT JOIN `#__assets` b ON a.parent_id = b.id WHERE b.id IS NULL

The correct result is only one row with name = 'root.1'.

olleharstedt · 2017-07-06T10:49:05Z

Thank you.

@csthomas Query checks out, only returns root.1.

olleharstedt · 2017-07-06T12:07:36Z

libraries/joomla/table/asset.php

 		// JTableNested does not allow parent_id = 0, override this.
 		if ($this->parent_id > 0)
 		{
 			// Get the JDatabaseQuery object
 			$query = $this->_db->getQuery(true)
-				->select('COUNT(id)')
+				->select('1')


This does not work on our system, for some reason. Query returns null, but with COUNT(*) it returns 1, as should be.

Using 1 AS x also works.

If only this change fix your problem then I do not understand it. What database do you have?

You said that:

SELECT count(*) FROM `#__assets` WHERE id = ?

return 1 and

SELECT 1 FROM `#__assets` WHERE id = ?

return null

Using 1 AS x also works.

OK I can understand the reason.
But I thought that such error exists only in sub-queries generated in mssql.
Removing offset and limit also should fix that, ex:
replace $db->setQuery($query, 0, 1) to $db->setQuery($query)

Removing limit and offset did not fix the issue.

mysql Ver 14.14 Distrib 5.5.55, for debian-linux-gnu (x86_64) using readline 6.3

Strange thing is, when I run the exact same code in a script using CLI, it works!

Do you know which database driver is used in assets table?

Oh... _db = JFalangDatabase Object

If you think this is a real joomla problem then you can create a PR and add alias to column as you mentioned. I will test it.

No, this is a Falang problem. They override the database driver and can't handle the query. But sure, if Falang won't fix it, it would be nice if the PR would be accepted.

A few improvements for assets, by default new article should have set…

08d10b2

… up inherited permissions

joomla-cms-bot added PR-staging Unit/System Tests labels Mar 3, 2017

csthomas mentioned this pull request Mar 3, 2017

Improve performance with assets #14279

Closed

joomla-cms-bot removed PR-staging Unit/System Tests labels May 1, 2017

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 1, 2017

rdeutz modified the milestone: Joomla 3.7.2 May 4, 2017

rdeutz modified the milestones: Joomla 3.7.2, Joomla 3.7.3 May 18, 2017

rdeutz removed this from the Joomla 3.7.2 milestone May 18, 2017

rdeutz merged commit fed63a2 into joomla:staging May 22, 2017

joomla-cms-bot added PR-staging Unit/System Tests and removed RTC This Pull Request is Ready To Commit labels May 22, 2017

csthomas deleted the some_assets_fix branch June 29, 2017 22:17

olleharstedt reviewed Jul 6, 2017

View reviewed changes

csthomas mentioned this pull request Jul 7, 2017

"SELECT 1" returns null #17000

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A few improvements for assets #14319

A few improvements for assets #14319

csthomas commented Mar 3, 2017 •

edited by joomla-cms-bot

ghost commented Mar 5, 2017

csthomas commented Mar 5, 2017 •

edited

ghost commented Mar 5, 2017

ghost commented Mar 5, 2017

csthomas commented Mar 6, 2017

ghost commented Mar 6, 2017

ghost commented Mar 6, 2017

csthomas commented Mar 6, 2017 •

edited

csthomas commented Mar 6, 2017 •

edited

ghost commented Mar 6, 2017

csthomas commented Mar 6, 2017

ghost commented Mar 6, 2017

csthomas commented Mar 6, 2017 •

edited

ghost commented Mar 6, 2017

csthomas commented Mar 6, 2017

ghost commented Mar 6, 2017

sanderpotjer commented Apr 30, 2017

ghost commented May 1, 2017

olleharstedt commented Jul 6, 2017

rdeutz commented Jul 6, 2017

csthomas commented Jul 6, 2017

olleharstedt commented Jul 6, 2017

olleharstedt Jul 6, 2017

olleharstedt Jul 6, 2017

csthomas Jul 6, 2017

csthomas Jul 6, 2017

olleharstedt Jul 6, 2017

olleharstedt Jul 6, 2017

olleharstedt Jul 6, 2017 •

edited

olleharstedt Jul 6, 2017

csthomas Jul 6, 2017

olleharstedt Jul 6, 2017

A few improvements for assets #14319

A few improvements for assets #14319

Conversation

csthomas commented Mar 3, 2017 • edited by joomla-cms-bot

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

ghost commented Mar 5, 2017

csthomas commented Mar 5, 2017 • edited

ghost commented Mar 5, 2017

ghost commented Mar 5, 2017

Using default user groups:

and work your Example:

csthomas commented Mar 6, 2017

ghost commented Mar 6, 2017

ghost commented Mar 6, 2017

csthomas commented Mar 6, 2017 • edited

csthomas commented Mar 6, 2017 • edited

ghost commented Mar 6, 2017

csthomas commented Mar 6, 2017

ghost commented Mar 6, 2017

csthomas commented Mar 6, 2017 • edited

ghost commented Mar 6, 2017

csthomas commented Mar 6, 2017

ghost commented Mar 6, 2017

sanderpotjer commented Apr 30, 2017

ghost commented May 1, 2017

olleharstedt commented Jul 6, 2017

rdeutz commented Jul 6, 2017

csthomas commented Jul 6, 2017

olleharstedt commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olleharstedt Jul 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csthomas commented Mar 3, 2017 •

edited by joomla-cms-bot

csthomas commented Mar 5, 2017 •

edited

csthomas commented Mar 6, 2017 •

edited

csthomas commented Mar 6, 2017 •

edited

csthomas commented Mar 6, 2017 •

edited

olleharstedt Jul 6, 2017 •

edited