Problem/Motivation

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?

Steps to reproduce

Use the module.

Proposed resolution

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.

Remaining tasks

Discuss.

User interface changes

None.

API changes

The config would change.

Data model changes

None.

CommentFileSizeAuthor
#6 content_access-3377809-6.patch9.66 KBgisle

Issue fork content_access-3377809

Command icon Show commands

Start 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:

About issue forks

Comments

Steven Jones created an issue. See original summary.

steven jones’s picture

steven jones commented

I thought I'd seen this issue in the queue already, but couldn't find it again, apologies if this is a duplicate.

steven jones’s picture

steven jones commented

I'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.

steven jones’s picture

steven jones commented
Status: Active » Needs review

There'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.

Steven Jones opened merge request !9

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
StatusFileSize
new content_access-3377809-6.patch9.66 KB

Due 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.

steven jones’s picture

steven jones commented
Status: Needs review » Needs work

So 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:

  1. Release 2.0.0 without this change
  2. Work on the change, getting the config into the third party settings collection
  3. Release a 2.1.0 that contains an upgrade path etc.

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?

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Related issues: +#3368266: unserialize() is insecure unless allowed classes are limited. Use a safe format like JSON or use the allowed_classes option.

Yes, 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.

steven jones’s picture

steven jones commented

Thanks @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 :)

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented

Thanks for offering to help out! I've added you as co-maintainer.

quadrexdev made their first commit to this issue’s fork.

quadrexdev’s picture

quadrexdev
Location Lutsk
commented
Status: Needs work » Needs review

I 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.

steven jones’s picture

steven jones commented

Amazing! Thanks so much.

quadrexdev’s picture

quadrexdev
Location Lutsk
commented
Version: 2.0.x-dev » 2.1.x-dev
Issue tags: +LutskGCW26

quadrexdev’s picture

quadrexdev
Location Lutsk
commented
Status: Needs review » Fixed

Updated tests and merged in 2.1.x

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.