The config for Content Access has serialized strings of data within, making it hard to read by humans, and machines like config_split.
Before we get to a 2.x stable release, would we be able to clean this up please?
Use the module.
I'm thinking we could split the config for each content type into it's own file? And then instead of having serialized arrays stuffed in, they could be proper mappings with sequences for each role.
Discuss.
None.
The config would change.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | content_access-3377809-6.patch | 9.66 KB | gisle |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
steven jones commentedI thought I'd seen this issue in the queue already, but couldn't find it again, apologies if this is a duplicate.
Comment #3
steven jones commentedI've got a patch in the works, and the config is great!
content access already sends the config loading through a couple of functions, so we only need to change out those places and it'll work really nicely.
Comment #4
steven jones commentedThere's some roughly working code for discussion.
I wonder if actually this should go as a 'third party' setting on the NodeType entity, and then let that handle the config storage for us etc.
Comment #6
gisleDue to recent changes MR !9 produces a merge error. I've rerolled it. Please find the reroll in the attached patch.
Would appreciate a review.
Comment #7
steven jones commentedSo thinking about this more, I really think this should be a third party setting on the NodeType entity, and then it handles all the storage for us, which removes a ton of code etc.
I really don't think my original approach of adding another config entity type was the right way to go here!
I'd suggest:
That then doesn't hold up the release, but then also reduces the maintenance burden going forward.
@gisle however, you're the module maintainer here, so what do you reckon, should we aim for being a third party setting instead?
Comment #8
gisleYes, I am the module maintainer (not because I am the most qualified, but because I need this module, and nobody else seemed to be willing to do the job). This is not an area where I claim to be an expert, so I have to rely to the judgement of others, such as yourself.
Lately, I've also become aware that serialized data may be insecure (see, for example, related issue #3368266: unserialize() is insecure unless allowed classes are limited. Use a safe format like JSON or use the allowed_classes option.). That's is another strong argument for moving the configuration from serialized data and into a separate settings file.
As for implementing it as a third party setting on the NodeType entity, I think that sounds as a better solution, and if it can be done, I'm prepared to wait for that.
As for issues holding back a full 2.0.0 release, they are tracked here: #3143952: [meta] Content Access release 2.0.0. I haven't added this issue as a blocker. The major one seems to be getting automated tests working again.
PS: I've looked at your profile page and it is impressive. I open to adding a co-maintainer to this project if you're interested.
Comment #9
steven jones commentedThanks @gisle, that makes sense!
Thanks for maintaining this module, it is much appreciated.
I'd be up for being a co-maintainer if you'd have me. It does seem like content access (and acl) need some love to bring them into the modern PHP and Drupal 9 world proper, and they can probably go back into hibernation :)
Comment #10
gisleThanks for offering to help out! I've added you as co-maintainer.
Comment #12
quadrexdevI worked on this. Please see the merge request - https://git.drupalcode.org/project/content_access/-/merge_requests/9
1. Moved "content_access_node_type" config part to node type third-party settings.
2. Updated schema.
3. Added update_N hook to move existing configurations.
4. Added test coverage for update_N hook.
Please review.
Comment #13
steven jones commentedAmazing! Thanks so much.
Comment #14
quadrexdevComment #16
quadrexdevUpdated tests and merged in 2.1.x
Comment #19
joegl commentedCould we add a note to the 2.1.0 release that the schema changed and possibly a change record? We had a couple issues on different sites with the moving of the configuration from content_access.settings to node.type configuration. On one multi-site we had content_access.settings ignored via config_ignore to allow per-site customizations, and the migration to node.type settings (which weren't ignored) dropped all customization across every site and exposed content that should have been private (and locked content away that should have been visible). I just want to hopefully help others avoid the same issues we experienced, and I don't think the title of this issue and call out in the changelog accurately captures what changed here.It appears this was called out in the release notes and I missed it.
Comment #20
steven jones commentedYeah, let me make that section of the release notes more obvious.
Comment #21
joegl commentedThanks Steven!