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

adds content_access_by_path module #745

Open
wants to merge 2 commits into
base: 3.x
Choose a base branch
from

Conversation

markconroy
Copy link
Member

Creating PR for the profile instead of LocalGov Drupal Core, see localgovdrupal/localgov_core#215


Thanks to Big Blue Door for sponsoring my time to work on this.

@finnlewis
Copy link
Member

Briefly discussing in Merge Tuesdays:

  1. would like to run this through our policy to check for bringing this into the localgov profile. Do enough councils want this by default etc.
  2. also I wanted to test and check on any potential security assumptions: if paths change for other reasons, might this give access to manage content by accident?

Tasks:

  • Finn to find the policy that Tech Group and Product Group agreed and a to docs site / link here.
  • Will to check with other councils on wants and needs.

@stephen-cox
Copy link
Member

Will has tried to ask content people about this, but has not had any responses.

@andybroomfield
Copy link
Contributor

What do I need to do to review?

@finnlewis finnlewis requested a review from willguv September 17, 2024 11:40
@andybroomfield
Copy link
Contributor

Looking at the issue, this is so it's avalible to people as part of the profile, but not installed by default.
So this PR passes in terms of adding the module, if that is aggreed can approve on that basis.

@finnlewis
Copy link
Member

Hi @willguv

Could we review the policy https://docs.localgovdrupal.org/governance/new-features-policy.html

and check in on point 1?

Also @andybroomfield want to check in on the security aspects. If paths change, does this change the access control?

@finnlewis finnlewis self-assigned this Jan 21, 2025
@finnlewis
Copy link
Member

I'm just taking a look at this again. It would be good to confirm if there are any assumed security features that would be bypassed by changes in the IA / path aliases.

@willguv did you get any input from content / product group discussions on the need for this from multiple councils?

@rupertj
Copy link
Member

rupertj commented Jan 21, 2025

I think it'd make more sense to add this to the project than the profile. (And I'm starting to think that about everything that's not actually required to run an LGD site, for the record...)

If you add this to the project, new installs get it and can choose whether to use it or not. If you add it to the profile, it'll get deployed to every LGD site out there, even if they don't want it. I'd really like to cut down the codebase on the sites I work on to just what they actually need to run.

@willguv
Copy link
Member

willguv commented Jan 21, 2025

Hi @finnlewis I've had very little back from content designers about this, so while this is probably needed by some it's not pressing

Also this isn't in any of the missions, either active or proposed - we've got a lot of concurrent work on, so think we need to need to deprioritise this

Sorry for the slight change of heart - hope this is OK for you and @markconroy

@finnlewis
Copy link
Member

I think it'd make more sense to add this to the project than the profile. (And I'm starting to think that about everything that's not actually required to run an LGD site, for the record...)

If you add this to the project, new installs get it and can choose whether to use it or not. If you add it to the profile, it'll get deployed to every LGD site out there, even if they don't want it. I'd really like to cut down the codebase on the sites I work on to just what they actually need to run.

That is a very good point @rupertj.

Adding fully optional modules to the project composer.json makes a lot of sense, as long as we're not enabling them on a default install. We're encouraging them as optional modules rather than baking them in, making it easier for them to be removed as required by each installation.

On the flipside, what is the problem with additional code that is not enabled? Drupal itself comes with lots of optional modules / code that we don't use by default. Our default installation does not install most of the optional localgov_ modules. What is the argument against having optional code or modules that are not installed / enabled?

@andybroomfield
Copy link
Contributor

Or revisit localgov_minimal #160

@rupertj
Copy link
Member

rupertj commented Jan 21, 2025

It's not the worst thing in the world, especially for a tiny module like content_access_by_path, but modules that are in the profile are downloaded multiple times for every single CI run the project makes, as well as every single CI run when people test and deploy their own sites.

Having unused modules hanging around is also a minor maintenance burden. They're trivial to keep up to date, but you still need to do it.

@willguv
Copy link
Member

willguv commented Jan 21, 2025

@rupertj an aside, but is it worth is reviewing what's in core and what's missing?

We review each time we want to add something, but we haven't looked at the whole thing in years. The proposed Refresh mission for this year could tackle this if we think it's not much work/ delivers some value

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.

6 participants