Skip to content
This repository has been archived by the owner on Aug 18, 2024. It is now read-only.

Split up OgAccess::userAccessEntity() #673

Merged
merged 9 commits into from
Aug 2, 2020
Merged

Conversation

pfrenssen
Copy link
Contributor

Fixes #672

@pfrenssen pfrenssen marked this pull request as draft July 29, 2020 13:04
@pfrenssen pfrenssen force-pushed the split-up-useraccessentity branch from ce84867 to 41ffac9 Compare July 29, 2020 13:24
@pfrenssen pfrenssen force-pushed the split-up-useraccessentity branch from 41ffac9 to 5f6d8a9 Compare July 29, 2020 13:34
This method was previously handling both user permissions and entity operations
but this was confusing and problematic. It is now split into two separate
methods with a clear scope: one is handling user permissions and the other
handles entity operations.
@pfrenssen
Copy link
Contributor Author

This is ready but since this is built on top of #671 I will keep this in draft until the other PR is accepted.

@pfrenssen pfrenssen marked this pull request as ready for review July 29, 2020 18:05
@pfrenssen
Copy link
Contributor Author

Setting back to draft since this needs more work. I am running tests on my "big" project and there is a regression. ::userAccessEntity() was being called from og_entity_access() with an entity operation, both on group content and group entities. It should only do this for group content from now on.

We have currently no full support for entity operation permissions on groups, only for the update operation (through the UPDATE_GROUP_PERMISSION GroupPermission). I will make a followup to add also a delete permission.

@pfrenssen pfrenssen marked this pull request as draft July 30, 2020 09:47
@pfrenssen
Copy link
Contributor Author

Created followup #674 but I had another look and we can still retain the functionality in scope of this, we simply need to check if we are handling a group entity and if this is the case we can translate update to UPDATE_GROUP_PERMISSION and delegate to ::userAccess(). It will be solved in a couple lines and there will not be a regression for now.

I propose to handle the test coverage for this in #674. We don't have the coverage right now and this would be out of scope.

…content.

This benefits og_entity_access() which relies on this to work for any entity.
@pfrenssen
Copy link
Contributor Author

pfrenssen commented Jul 31, 2020

I have added the code which solves the regression and I think this is now complete.

I will keep this in draft until I have finished work on #674 - that issue will complete the entity operations for group entities by adding the delete operation. Most importantly it will also add the necessary test coverage which I think is out of scope for this PR.

I also want to run the extensive test suite on my work project that has discovered this regression.

@pfrenssen
Copy link
Contributor Author

I am near finishing work on #674. Here is the coverage for the regression on hook_entity_access(): https://github.com/Gizra/og/pull/681/files#diff-103d019edfa64fc25b94e05082e8bba8R332

This is now ready for review.

@pfrenssen pfrenssen marked this pull request as ready for review July 31, 2020 16:34
@pfrenssen
Copy link
Contributor Author

@MPParsley Thanks for reviewing! I addressed the remark. I will wait for test results to come back green before merging. I can then "unlock" the next PR.

This OgAccess refactoring is coming to an end soon, I have finished work on the final 2 PRs (#681 and #684) and I am now working on integrating them into my project so I can run our extensive test suite.

I can also still as part of this work write the release notes for the next release since this is breaking backwards compatibility. We can still break B/C as long as we're in alpha but I would like to inform our users so that they will know what to expect when updating.

@MPParsley
Copy link
Collaborator

Thanks for the update!

I'm mostly worried about the impact on the ongoing PRs for beta blockers (e.g. #231, #242, #570, ...).

@pfrenssen
Copy link
Contributor Author

pfrenssen commented Aug 2, 2020

The impact is due to having split the entity access check into separate checks for user permissions and entity operations. So existing code that is checking entity operations will need to call the other method now.

Here is an example how this now looks in my project code: (ref. https://github.com/ec-europa/joinup-dev/pull/2184/files#diff-f26d2164853bef0da7d08750148704f5L86)

-    return $this->ogAccess->userAccessEntity('create', $this->createNewAssetRelease($rdf_entity), $account);
+    return $this->ogAccess->userAccessEntityOperation('create', $this->createNewAssetRelease($rdf_entity), $account);

Also if people are implementing hook_og_access_alter() and are altering the results expecting entity operations to be passed in, they will need to use hook_og_access_entity_operations_alter() now. Example: https://github.com/ec-europa/joinup-dev/pull/2184/files#diff-0c9ed0667150da0d6130597e6bb8789bR179

@pfrenssen pfrenssen merged commit 8029b4b into 8.x-1.x Aug 2, 2020
@pfrenssen pfrenssen deleted the split-up-useraccessentity branch August 2, 2020 19:08
MPParsley added a commit that referenced this pull request Aug 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split up OgAccess::userAccessEntity() into two methods
2 participants