Add owner to the BlockContent entity type

Here's a patch based on #15 working on Drupal 10.1.6

Patch #15 is incorrect, the entity key is incorrect and it's not using the EntityOwnerTrait

I've updated the IS with all steps to get this across the line. The first being a rebase onto 11.x.

Split authoring information + summary JS out into ✨ [PP1] Implement authoring information form element and summary for Block Content Active we can deliver the bulk of functionality easier that way.

Hiding all patches, MR up with a reroll onto 11.x + using EntityOwnerTrait + an install hook for the field def.

https://git.drupalcode.org/project/drupal/-/merge_requests/5500

Should be good to review.

Seems like a good first step to getting authored on field add. Though was initially testing on umami where they had those fields on the block_content.

Applied locally and ran the updb which both ran fine. I see in the DB there is a UID column now for block_content_field_data

LGTM

Though was initially testing on umami where they had those fields on the block_content.

It looks like that's the translation author, although I'm not sure how that interacts with the owner field!

Left a couple of comments on the MR.

Feedback addressed and back to green (after a random fail).

Believe feedback has been addressed and look forward to this + the UI for it being add :)

I'm triaging RTBC issues → . I read the IS and the comments. I didn't find any unanswered questions. But is does look like a follow up is needed for this comment. I am tagging for that.

I updated credit.

Leaving at RTBC.

acbramley → changed the visibility of the branch 3038365-add-the-entityownerinterface to hidden.

acbramley → changed the visibility of the branch 11.x to hidden.

Needs a reroll

I think that bug will be fixed by 📌 Automatically set revision user/log information/created time on entity revisions Needs review but I'll need to double check.

Does seem issue in #36 should address that comment. Restoring RTBC after reoll

Renamed the hook_update_N() to reflect it landing in 10.3.0, otherwise looks good so committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

acbramley → closed merge request !5500

There's good research and discussion going on in 📌 Investigate performance of block_content_post_update_set_owner Needs review but it's not a quick fix, so I've gone ahead and reverted the commits here - we can recommit with the optimized update path once that's decided.

I had a thought about this when I saw this issue in the RTBC queue. What does this mean for user cancelling. For nodes we offer to delete the user's content or move it to the anonymous user.

With some content blocks deleting them would break your site. This feels a tricky issue.

acbramley → reopened merge request !5500

I had a thought about this when I saw this issue in the RTBC queue. What does this mean for user cancelling. For nodes we offer to delete the user's content or move it to the anonymous user.

Anonymous user seems reasonable. Deleting does not.

User cancelling is pretty broken overall, we still have 🐛 comment_user_cancel() doesn't implement a batch Needs review open.

It's not clear to me if there's anything to do here wrt. user cancelling? In any case I've merged the changes from 📌 Investigate performance of block_content_post_update_set_owner Needs review into this MR.

Adding a new field definition can have an initial value. We can initialize them to 0 so that they are not empty. or we could leave them as null and set an author the next time they are being saved.

I wondered about this, and I can't really see a downside. Is there any issue with users being able to edit blocks by different owners? I guess our permissions aren't that granular for blocks so maybe not?

> I guess our permissions aren't that granular for blocks so maybe not?

I was going to ask about this too... is there a follow-up to add the own permissions?

Adding a new field definition can have an initial value.

There's also BaseFieldDefinition::setInitialValueFromField(), which in this case would set the owner of each revision to the value of the `revision_user` field.

> There's also BaseFieldDefinition::setInitialValueFromField(), which in this case would set the owner of each revision to the value of the `revision_user` field.

True, but I would be surprised if that works as it's not stored in the same table. I think I tried something like that before in a different use case.

Yeah I tried #51 last night and it does not work. Maybe it could be fixed to work... but that is likely to be super complex.

is there a follow-up to add the own permissions?

I would argue against adding own permissions - this could be done in contrib. It adds serious cacheability issues as seen in 🐛 MediaAccessControlHandler update/delete access caching is not correct Needs work

Would be good to get a response re #48 and now the new ideas about whether or not we need the post update.

I was just about to apply this to our client site and rip out our custom implementation but I want to hold off until this is committed again so I'm not reworking again.

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.

Rerolled and squashed the MR against 11.x - now green again.

We still need to decide whether we keep the upgrade path or not. IMO the code is there and working so it seems like it'd be good to keep but if people feel otherwise then I'm not too fussed tbh...

Posted about this in slack #block-content and only hold up seems to be around the post_update hook. Since it's already been added and is functional I don't think we need to remove.

This is not committable without the comment on the post-update hook being addressed one way or another. It's actually a pretty important question in terms of data integrity and user expectations.

I also think this is the type of change that merits signoff from the maintainers of the subsystem (also possibly framework managers, but let's start with the subsystem).

Meanwhile, posted a couple small points of my own feedback on the MR, so NW instead of NR for those to be addressed.

Thanks!

And probably RM signoff eventually to validate the community consensus about the post-update hook, I guess, but I'd like to hear from subsystem contributors first. :)

That would be @acbramely and myself so maybe needs to be framework manager as @larowlan is both.

Sorry, one more thing... before commit, I'd like clear confirmation that the performance regression has absolutely definitely been addressed. Thanks!

The new patch uses a limit and sort per #3425888-6: Investigate performance of block_content_post_update_set_owner → which in theory should resolve the performance issue.

I like the 'let's just make them all anonymous (uid 0)' idea because none of block content's permissions (see \Drupal\block_content\BlockContentAccessControlHandler::checkAccess) take the owner into account. But given the update hook is written, I think we should test the performance. I will ask @luke.leber if he can retry this patch with the same site he saw the issue on previously. If it performs OK, I think we may as well leave things in the best state rather than offloading the question to a contrib solution.

which is acbramley and smustgrave saying "meh" apparently

@xjm I feel like this must have been a driveby comment? Did you read through everything in this issue and the others? This is quite rude to say. I've worked a lot on this issue, including the original post update hook that got reverted and working to make it more performant. I've manually tested this countless times on large sites.

In #57 I said I'm not fussed if we have it or not, I just want to get this issue in as it's a good improvement and had already been committed before.

I like the 'let's just make them all anonymous (uid 0)' idea

I think that's a decent approach if we deem this update hook too heavy, although for me it seems like something core absolutely needs to be able to support. Direct db queries are the most performant way to do this (that I can think of), if even that is too heavy what happens when an issue comes along that must have an update hook like this? We need a scalable way to set entity values in upgrade paths.

Also, I feel like setting everything to uid 0 may be confusing to users. Block content entities already have a revision user, if the author gets added and its anonymous that could be confusing in the UI?

@acbramley, my apologies, I did not mean to sound critical at all. I was documenting that both you and @smustgrave don't have strong feelings either way about the post-update hook (you feel "meh" about it), and so therefore we needed to escalate it to a tie-breaking role (Lee in this case). Sorry again for how it came across!

Timing on !5500 on 19621 blocks, 24290 revisions, and a batch size of 50 (default).

real    0m19.932s
user    0m11.936s
sys     0m0.955s

~20 seconds is a lot more acceptable than 30 minutes! What a difference the Database API makes versus the Entity API!

Thanks for your patience on this issue.

I discussed this issue at Dev Davs with @catch and @daffie following discussion with @acbramley in Slack.

The new performance is great; however, the downside of the approach currently being used is that it won't work on no-SQL databases. We discussed this with @daffie and he will propose a related issue that will allow core updates to better support an SQL case and a no-SQL case.

We also discussed the proposal to set it to 0 as @Berdir suggested. We're concerned that this could have unintended side effects in edgecases or specific site configurations.

If the data validation allows it to be set to NULL, that would be ideal. Then we can add code that happens on entity save to set a given block's author based on the first revision, so that it happens as-needed rather than in a big batch. So that's the approach we recommend (so long as it is feasible).

Meanwhile, this issue has a data model improvement that should have a change record and maybe a release note. (We'll decide after the final approach whether to use the release note, but having a good release note will help us think about the impacts on site owners, code, etc. as well as edgecases.)

Leaving "Needs release manager review" pending the new approach. Thanks!

xjm → credited daffie → .

Saving some credits.

I agree 0 feels risky, not for core but in case contrib is doing something weird.

NULL seems OK if we could do it, but not sure what the implications of that are.

So checking whether we're running on a relational database or not then doing the direct update if we are seems OK compared to all the other options. I'd be OK with hard-coding mongodb in the update here if we have the follow-up to add a connection class method or similar. Or if that issue is easy to get done, we could do that first then check here (with otherwise exactly the current logic in the patch).

20 seconds for thousands of blocks is more than good enough for performance of the update so that bit is great now.

NULL seems OK if we could do it, but not sure what the implications of that are.

Doing some testing locally - I don't see any issues with leaving it NULL, the owner field is already nullable and the default value is null. That means when we install the new field (in block_content_update_10301) all existing rows get NULL as the uid column value. So step 1 is taken care of automatically :)

Next step was to test what currently happens with existing blocks if you edit them without an author, I expected the defaultValueCallback from EntityOwnerTrait to kick in and set the owner to the current user, but that does not happen on update, only on insert. IMO that's good - one less thing for us to worry about overriding.

(FYI That does not happen for revision_user - I reported this somewhere else but forgot about it, Block Content's revision_user is not set on the initial revision only on subsequent revisions. Node acts correctly so there must be some custom code doing that rather than using the default value callback. Tested MenuLinkContent as well and that has the same issue. I think 📌 Automatically set revision user/log information/created time on entity revisions Needs review will cover this)

Next is to implement a preSave which sets the owner based on the initial revision's revision_user (which may be empty for some blocks because of the above, not much we can do about that though). As I'm writing this though I realise this can't use the same database queries though...

This will most likely be my last push on this issue. Someone else will have to pick up the torch from here on out as I've lost interest in getting it across the line due to all the rework. Unfortunately that probably means this issue is dead in the water.

I think we're going to have quite a few issues with this new approach:

1. The update path test will probably fail because the author will be set by the preSave
2. It's impossible to test that preSave because the author will be set for new blocks by the default callback
3. It's really ugly IMO.

https://www.drupal.org/node/3457732 →

Appears to have a functional test failure

1)
    Drupal\Tests\block_content\Functional\Update\BlockContentUpdateTest::testAddAndSetOwnerField
    Failed asserting that null matches expected '4'.
    
    /builds/issue/drupal-3038365/core/modules/block_content/tests/src/Functional/Update/BlockContentUpdateTest.php:74

This would be very awkward for people already using the patch, but #1233394: Agree on a property naming pattern → reminded me that we shouldn't be adding new fields with the old combined property/field names like uid anymore. Back then that issue proposed user_id, but I think just naming it owner would make more sense.

This issue is stalled on worse things than what the field is named 😅 I have no preference but it does seem redundant to change it at this point, just making it harder for people using it already

Agree that the ship has likely sailed with such naming convention standardizations as the landscape exists.

The time to decide such things was before there were literally billions of records and tens of thousands of custom sites depending on certain field names.

Any changes at this point would likely require a brand new way to performantly migrate huge volumes of records to new columns and would likely require maintaining redundancies between multiple major Drupal versions to prevent disastrous upgrades.