Problem/Motivation

The following procedural functions need to move into OO code:
node_access_needs_rebuild
node_access_rebuild
_node_access_rebuild_batch_operation
_node_access_rebuild_batch_finished

We should create a new service for this for API around rebuilding node access.

Steps to reproduce

N/A

Proposed resolution

TBC call this NodeAccessRebuild with rebuild and needsRebuild methods.

Deprecate node_access_needs_rebuild and node_access_rebuild and call out to new service

Remove _node_access_rebuild_batch_operation and _node_access_rebuild_batch_finished as these are internal (prefixed with _)

Remaining tasks

Do it

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#38 3533299-nr-bot_g3z6u8s7.txt91 bytesneeds-review-queue-bot
#35 3533299-nr-bot_vy8jt16h.txt91 bytesneeds-review-queue-bot
#23 3533299-nr-bot_ovaf2nel.txt91 bytesneeds-review-queue-bot
#21 3533299-nr-bot_5cpnr1qs.txt91 bytesneeds-review-queue-bot
#19 3533299-nr-bot_1mfazmlx.txt91 bytesneeds-review-queue-bot
#17 3533299-nr-bot_63rr57dz.txt91 bytesneeds-review-queue-bot
#13 3533299-nr-bot_a3c5isqh.txt91 bytesneeds-review-queue-bot
#11 3533299-nr-bot_xgzknrah.txt91 bytesneeds-review-queue-bot
#9 3533299-nr-bot_cbg99h94.txt91 bytesneeds-review-queue-bot
#7 3533299-nr-bot_5nnrq4ob.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3533299

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

acbramley created an issue. See original summary.

acbramley’s picture

acbramley commented
Parent issue: » #3532197: [meta] Deprecate node.module functions
virag.jain’s picture

virag.jain commented
Assigned: Unassigned » virag.jain

virag.jain opened merge request !12656

acbramley’s picture

acbramley commented
Status: Active » Needs work
virag.jain’s picture

virag.jain commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3533299-nr-bot_5nnrq4ob.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

acbramley’s picture

acbramley commented
Assigned: virag.jain » Unassigned
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3533299-nr-bot_cbg99h94.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3533299-nr-bot_xgzknrah.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3533299-nr-bot_a3c5isqh.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
mstrelan’s picture

mstrelan commented
Status: Needs review » Needs work

Left some feedback

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

Feedback addressed

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3533299-nr-bot_63rr57dz.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3533299-nr-bot_1mfazmlx.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

Squashed and rebased.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3533299-nr-bot_5cpnr1qs.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3533299-nr-bot_ovaf2nel.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
dcam’s picture

dcam commented
Status: Needs review » Needs work

I found a couple of things and made comments on the MR. And the deprecation version strings need to be updated. And it needs to be rebased. So I'm setting this to Needs Work.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

Feedback addressed.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Sorry to kick this back.

I think private properties have caused some serialization issues.
Also batch callbacks don't need to be static anymore so we can finish that conversion too.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

Actioned feedback except the batch builder stuff

nicxvan’s picture

nicxvan commented

#3575096: Remove callable type from BatchBuilder ::addOperation and ::setFinishCallback

acbramley’s picture

acbramley commented

This should be ready to go now

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Sorry to kick it back again, but the reason we were waiting was so the methods didn't need to be static and could use DI.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

Thank you! This looks great!

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3533299-nr-bot_vy8jt16h.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

acbramley’s picture

acbramley commented
Status: Needs work » Reviewed & tested by the community
nicxvan’s picture

nicxvan commented
Related issues: +#3566536: [meta] eliminate core .module files
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3533299-nr-bot_g3z6u8s7.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

acbramley’s picture

acbramley commented
Status: Needs work » Reviewed & tested by the community

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 31e1980 and pushed to main. Thanks!

  • alexpott committed 31e1980b on main 
    task: #3533299 Deprecate node access rebuild functions

By: acbramley
By...

alexpott closed merge request !12656

alexpott opened merge request !15619

  • alexpott committed b5d0936a on main 
    Revert "task: #3533299 Deprecate node access rebuild functions"

This...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » main
Status: Patch (to be ported) » Needs work

Arghh... we did not remove all the references to node_access_rebuild() in docs. Reverted.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

Okay I've re-opened the main branch and then fixed all the code comments mentioning the old methods. Also applied the same fixes to the 11.x MR.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

The commit to review for the comment changes is https://git.drupalcode.org/project/drupal/-/merge_requests/15620/diffs?c...

alexpott reopened merge request !12656

alexpott opened merge request !15620

alexpott closed merge request !15620

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

Sorry about that!

I compared both MRs the only differences were the autowiring services and 11.x HelpHooks class didn't have a constructor yet on 11.x.

I pulled down the main branch and searched the four functions, the only remaining mentions are in the deprecated function docblocks.

I mistakenly thought node_access_needs_rebuild was referenced in the new service still, but @alexpott pointed out it's actually referencing the state key.

I think this is ready now.

I also reviewed all of @alexpott's commits, injection for the form, the mentioned services change, comments and the backport, they all look good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 3a0d690 and pushed to main. Thanks!
Committed 25e0b0b and pushed to 11.x. Thanks!

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.

  • alexpott committed 25e0b0b5 on 11.x 
    task: #3533299 Deprecate node access rebuild functions

By: acbramley
By...

  • alexpott committed 3a0d690d on main 
    task: #3533299 Deprecate node access rebuild functions

By: acbramley
By...

godotislate closed merge request !12656

godotislate closed merge request !15619

godotislate’s picture

godotislate
he/him
commented
Status: Fixed » Needs work

The 11.x PHP 8.3 jobs (Postgres, mysqli, and sqlite) are failing, and it looks like it's an issue with the $nodeAccessRebuild property in RebuildPermissionsForm being readonly: https://project.pages.drupalcode.org/-/drupal/-/jobs/9609293/artifacts/s...

Basically, with DependencySerializationTrait, the service property is being re-initialized from the container in __wakeup(), which is causing an error because it is readonly and can not be set outside the constructor.

Not sure why this isn't affecting PHP 8.5, because I haven't found anything about fixing that readonly properties can be set in __wakeup(). We do have an open issue about this: #3424344: Allow DependencySerializationTrait to work with readonly properties, and #3548971: Replace PHP soft-deprecated __sleep()/__wakeup() with __serialize()/__unserialize() is somewhat related, because it seems readonly properties can be set in __unserialize

godotislate opened merge request !15679

godotislate opened merge request !15680

godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Fixed

Actually, there are multiple tests failing because of separate issues, so opened #3588558: Test failures on 11.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.

godotislate closed merge request !15680

godotislate closed merge request !15679

Status: Fixed » Closed (fixed)

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