Errm, this is my first pull request and it seems to have gone slightly wrong.

I'm only wanting to make the changes in 298dae1.

I didn't intend all those others :(

Errm, this is my first pull request and it seems to have gone slightly wrong.

I'm only wanting to make the changes in 298dae1.

I didn't intend all those others :(

Maybe you have chosen to make the pull request against a different base branch than the branch based on which you have created the branch for your PR? This pull request currently is against the 4.3-dev branch. You can change the branch on GitHub by going to the pull request and then using the edit button right beside the title, then there is a way to change the base branch.

I'm fine (as far as I know) with the 4.3 branch, but the three checks listed, one of which has failed, are nothing to do with me, are they?

I'm fine (as far as I know) with the 4.3 branch, but the three checks listed, one of which has failed, are nothing to do with me, are they?

They are PHP code style errors reported in one of these tools, and I think they are related to your PR: https://ci.joomla.org/joomla/joomla-cms/68979/1/8

Follow the above link and then click on PHPCS to see the log.

Ah, ok, I see. Yes, there are code style errors there.
How do I fix that for this PR?

Ah, ok, I see. Yes, there are code style errors there.
How do I fix that for this PR?

Edit the file and use spaces and not tabulators for indentation. 4 spaces for each level of indentation. When working on a local clone, commit the changes and then push them to remote. Or edit directly on GitHub.

@MarkRS-UK first of all: thank you for your PR, glad to have you on board :)

From a security perspective I have one remark: you have modified the viewing permissions, but not the storage permissions. In it's current state, a user with limited view permissions for just one fieldset will be able to store parameters in other, invisible fieldsets too. That should be fixed.

Hi @SniperSister , thanks for your comment.

I've had a look at the save method for component in com_config.
As far as I understand it, only displayed configuration data is available for saving, and that is controlled by the ACL system. The user still has to have the admin action provided to be able to access any preference data.
Does my change provide any greater possibility of access violation than is currently available? If so, please let me know where so I can fix it.

As far as I understand it, only displayed configuration data is available for saving

Nope :) You can manipulate the saving request and add additional configuration flags that aren't displayed.

How does the current code prevent that?

How does the current code prevent that?

It doesn't because it's not necessary: If you can access the preferences you can access all of them - but your PR wants to change that.

@MarkRS-UK Independently from the above discussion: As this would be a new feature, this PR should be made for the 5.0-dev branch. Could you rebase your PR to that branch? Let us know here if you need help with that.

Yup, good point @SniperSister.
I've made changes to take account of that in ComponentModel.php, and it looks like my guess of how to include that was right.

@richard67 , thank you for your help previously. Err, ok, as soon as the checks are complete I'll move it to 5.0... When is 5.0 scheduled to be released?

@richard67 , thank you for your help previously. Err, ok, as soon as the checks are complete I'll move it to 5.0... When is 5.0 scheduled to be released?

@MarkRS-UK See https://developer.joomla.org/news/909-joomla-5-0-alpha-4-your-last-chance-to-add-a-feature.html :

Final release: 17th October 2023

But feature freeze will already be with beta 1, which is scheduled for 5th September.

Will it cause trouble if I change the base branch to the 5.0 dev before these checks have finished? It's taking ages to finish/fail.

@MarkRS-UK Now after the rebase, your PR shows many changed files on GitHub and conflicts for some files. Don't get nervous. The reason is that the 5.0-dev branch was not up to date with the latest changes from 4.0-dev, but your branch was, so your branch is a bit in advance now. This will be fixed when the next upmerge from 4.0-dev via 4.4-dev to 5.0-dev has been made by maintainers or release managers. After that it should be possible to update your branch to latest 5.0-dev here with a button. If that is not the case and there will still be conflicts shown, then ping me to help. But for now I'd say let's wait for the next upmerges.

Thanks for that @richard67.
I'll keep a (relaxed) eye out for developments. Nice to see it's passed all the required checks in the meantime :)

Hey @richard67 , is this alright? It looks like there's been an upmerge but it still shows a conflict. Is that a problem I need to do something about?

@MarkRS-UK Unfortunately there is still a merge conflict.

The reason is that the file "plugins/content/loadmodule/loadmodule.php" from Joomla 4 has been moved to "plugins/content/loadmodule/src/Extension/LoadModule.php" and has been refactored with pull request (PR) #40561 . This makes the conflict being not trivial, so GitHub doesn't show me a button to easily solve it online. You can resolve it in your local clone by reverting all your changes in file "plugins/content/loadmodule/loadmodule.php", i.e. accepting the deleted file from 5.0-dev, and then applying your changes to the "plugins/content/loadmodule/src/Extension/LoadModule.php".

But maybe it would be easier for you when you create a new PR based on 5.0-dev as replacement for this one.

Otherwise, if you don't want to do that and can't solve the conflict yourself, I could try to help tomorrow or on weekend.

@richard67, I haven't touched that file, but you probably realise that.

Create a new PR based on 5? I've edited this one to merge with 5, is that not enough?
Can I sync my fork here on GitHub with 5.0 and if so, will that be enough?

@richard67, I haven't touched that file, but you probably realise that.

Yes, I just see. Sorry for the confusion.

Create a new PR based on 5? I've edited this one to merge with 5, is that not enough? Can I sync my fork here on GitHub with 5.0 and if so, will that be enough?

Not sure. I will see what I can do later tonight after my paid job, or tomorrow.

Many thanks. Yeah, paid jobs are important :)
I'm away for a couple of days from tomorrow noon but will get straight back here when I get back if I haven't heard before.

@MarkRS-UK I was able to solve the conflict. Now the PR still shows more changes because your branch was in advance to 5.0-dev, but that will be fixed with the next upmerge, of which I assume it will happen soon.

Many thanks @richard67 .
Looks like I best keep an eye on this this evening, checks failing??? Ooh-errr! :)

Many thanks @richard67 .
Looks like I best keep an eye on this this evening, checks failing??? Ooh-errr! :)

The failing checks are not related to your PR but to changes which come from the 4.3-dev branch and having more detailed code style checks in 5.0-dev. That will also be fixed with the next upmerge, which I expect to happen in the next days.

If there are no conflicting files, then is not necessary to regularly update the branch as it will resets test results that will have to be manually adjusted by one of the maintainers.

I do it when this page shows that the branch for this PR doesn't match the dev5.0 branch any more. Is that not the right thing to do?

It is ok to be out of sync. It will be updated before being merged after 2 successful tests. So updating the branch before that is unnecessary.

@Quy , I'm now confused. The branch is out of date now, the page shows there have been 3 successful checks. Is this in line with what you said above or do I need to update the branch manually?

It is ok to be out of date as long as there are no conflicting files which GitHub will tell you. Your PR it will be outdated as soon as another PR is merged. After 2 successful tests to your PR, then the branch will be marked as RTC (Ready to Commit) and updated. Until then, no need to keep manually updating.

This pull request has been automatically rebased to 5.1-dev.

Whilst that sounds nice @HLeithner, does that mean it's not going to go into the 5.0 release?

Whilst that sounds nice @HLeithner, does that mean it's not going to go into the 5.0 release?

yes, all feature pull requests have been rebased on 5.1 and all bugfix pull requests are rebased to 4.4

Whilst that sounds nice @HLeithner, does that mean it's not going to go into the 5.0 release?

It still needs testing and as its a feature the maintainers have to decide if it will be accepted

Ok, thanks both. I should'a said "is it not being considered for the 5.0 release". However, point taken.

This pull request has been automatically rebased to 5.2-dev.

We discussed this yesterday in the weekly maintainers meeting. Honestly, we don't see the use case or the need for something like this.

I moved it to the next meeting, because we thought the implementation only hides it and doesn't take care about save. So at least I want us to discuss it with all information and the correct state.

Stay tuned.

Thanks for considering it. Here's my use case.

I'm developing an extension for a volunteer group organising music a festival.
There are many people involved with many different areas of responsibility.
Various groups require component level data; for example ticket prices, marketing information, information distribution (by e- and snail-mail).
This is why I want to allow different groups access to different preferences. We don't want the accountants accessing the marketing preferences, for example.

If I'm right about the code required, this is a tiny change for significant increased functionality.

I would guess this is a step away from "normal" CMS (not just Joomla) use of "many clients one administrator". I have many administrators. Here we have many people working on projects. I think it facilitates building component that move a website away from something that helps with one part of a business to a core part of a business within a single component.

What do you think now? Is this something that came into your considering?

Reading your example this does sound like the approach taken by one of the joomla component builders where everything has its own permission. So it is achievable now for your own components just not for core. Having seen components generated with separate permissions for everything it really makes things very complex to use.

Thanks for that @brianteeman.
They managed to do it using the standard J! preference system, or they built their own?

Sorry I dont know. I just rememember that when I looked at a component built with it the permissions page was enormous

Thanks for helping me with this @brianteeman, but I'm sorry to say I don't follow.

It's not that there would be a huge number of permissions, but that the visibility of permissions is controllable.
The existing permissions code allows, even encourages, grouping of permissions into related types, each group is displayed in a tab.
I envisage (& intended to code) a situation where access to those tabs is a function of the user's group.

The (eg) accountants shouldn't be able to see preferences that are on the marketing tabs, and vice versa, or whatever.

It allows the extension developer to add a line to access.xml for each permission tab. I s'pose that could be a lot of lines, though that wouldn't be very sensible.

Do you remember what the extension was that you saw?

Today meeting decision is to merge this PR if we have a test case (maybe update a core component?) and proper documentation maybe you can update https://manual.joomla.org/docs/next/general-concepts/acl/ with the full access.xml documentation?

thanks

Just remembered this #24299

Maybe that would be a good example to create and document?

Thank you for the attention @HLeithner and the suggestion @brianteeman.
Yes, I'm up for doing those things. I'll post back here when I've finished or if I get stuck.

Documentation PR submitted

@brianteeman , I now understand better your concern about "everything having its own permissions". That isn't the case with this PR. It should perhaps have been called "Control access to GROUPS OF component preferences individually", as it is controlling preference tabs, not individual preferences.

However, there seems to be some problem with this as Harald tells me the team would rather control individual preferences. I agree with you on this, that that would be too clunky.

This pull request has been automatically rebased to 5.3-dev.