Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#56 | 1997262-56.patch | 7.53 KB | jibran |
#56 | interdiff.txt | 1.55 KB | jibran |
#53 | 1997262-53.patch | 7.53 KB | jibran |
#50 | custom-block-1997262-50.patch | 7.5 KB | tim.plunkett |
#44 | custom-block-1997262-44.patch | 7.54 KB | tim.plunkett |
Comments
Comment #1
chertzogYAY! my first non docs Drupal 8 core patch that actually does something!
Comment #2
aspilicious CreditAttribution: aspilicious commentedThis will remove the local task. I guess there isn't a button now?
Please don't remove the todo
Hmm your requirement check doesn't match the original entity_access delete check.
Not sure how that works with the new routing api.
Comment #3
chertzogI was using #1946438: Convert confirm_form() in openid.pages.inc to the new form interface as a guide.
1.) There is still a delete link on the block placement page as a dropdown link - below configure.
2.) Where would the todo go since the entire function is being removed?
3.) I changed the permission to match the other custom block administration routes in the custom_block.routing.yml file. As i figured those were better since they are using the new routing system.
Comment #4
aspilicious CreditAttribution: aspilicious commented1) Yeah but the local action is gone. Which is a regression. No?
2) The function is not gone the function is moved to a function inside a class. We still need to add code (in the future) to delete all the configured instances.
3) You're incorrect I think. Entity access does more than just checking the permission you added. If the entity access callback only checks for that permission this should be ok in theory for now. Although it would be better to have the entity_access callback as it can be safely edited later on and the routes will be updated automagicly...
In the end I'm prety sure you can't change the access callback based on some examples. Not everything in core is equal :).
Comment #5
kim.pepperFixed the issues in #2 and added in the correct form parameter names to fire the entity ParamConverter. Also added the explicit conversion options in the route config.
Comment #7
larowlanRelated #1994146: WSOD after deleting a custom block content entity
Comment #8
larowlanThis wasn't added back in the new Form
Not needed, no conversion
custom_block.delete?
This isn't an instance, just a content entity
Existing text was "Are you sure you want to delete %name", hence the failing test.
$custom_block, not $block
The custom block entity
Original message was 'Custom block %label has been deleted.'
Comment #9
nick_schuch CreditAttribution: nick_schuch commentedHere is a patch with the fixes identified in #8.
Comment #10
larowlanAssuming bot agrees
Comment #12
larowlanProvides a confirmation form for deleting a custom block entity.
Comment #13
nick_schuch CreditAttribution: nick_schuch commentedDam "?"! Here we go and the grammar update to.
Comment #14
larowlanif its alright with the bot
Comment #15
larowlanThis will collide with #1994146: WSOD after deleting a custom block content entity, that is critical so should go in first.
Comment #16
andypostLet's get #1994146: WSOD after deleting a custom block content entity in first and then add test
Is there really deletion of instances happen? unit test of the form?
Any reason for converters?
Comment #17
nick_schuch CreditAttribution: nick_schuch commented1) Is there really deletion of instances happen? unit test of the form?
Will look into it.
2) Any reason for converters?
The converter was removed in #9. Is there something Im missing?
Comment #18
andypostYep, converters are gone but:
I mean this @todo is not needed if deletion of instances would have a test
Comment #19
andypostNeeds re-roll after #1994146: WSOD after deleting a custom block content entity
Comment #20
larowlanre-rolling
Comment #21
larowlanRe-roll
Comment #22
larowlanComment #23
andypostRTBC if bot green
Comment #25
nick_schuch CreditAttribution: nick_schuch commented#21: custom-block-delete-form.22.patch queued for re-testing.
Comment #26
naxoc CreditAttribution: naxoc commentedComment #27
nick_schuch CreditAttribution: nick_schuch commented#21: custom-block-delete-form.22.patch queued for re-testing.
Comment #28
nick_schuch CreditAttribution: nick_schuch commentedIf comes back green will RTBC as per #23.
Comment #30
larowlanStraight re-roll
Comment #32
andypostShould be build on top of EntityConfirmFormBase also not sure that form name should be defined
Comment #34
andypostFor NG-entity. tests are green locally
Comment #35
larowlanNo idea what those tags were for, we have tests
Comment #36
tim.plunkettNo reason to override the one coming from EntityConfirmFormBase
Otherwise it's RTBC
Comment #37
twistor CreditAttribution: twistor commentedSo the edit form is showing up on the delete page.
Comment #38
andypostAgreed, if there's no tests that alters the form
Comment #39
tim.plunkettLooks good to me!
Comment #40
twistor CreditAttribution: twistor commentedThis is what I see when I try to delete a custom block with this patch.
Comment #41
naxoc CreditAttribution: naxoc commentedThis needs a re-roll.
Like twistor I am also confused about the form. That does not look right.
Comment #42
naxoc CreditAttribution: naxoc commentedWoops. Sorry. Does not need a reroll - but I need glasses.
twistors point is still valid though.
Comment #43
andypost@twistor please provide a steps to reproduce, I can't reproduce this
Looks like it different bug of this class.
Not sure why fields are displayed here
Comment #44
tim.plunkettI've solved that before, I'm moving the fix up a level.
Comment #45
andypostI think it's good to go
Comment #46
xjm#44: custom-block-1997262-44.patch queued for re-testing.
Comment #48
andypostneeds reroll
Comment #49
tim.plunkettComment #50
tim.plunkettStraight reroll.
Comment #51
jibran#50: custom-block-1997262-50.patch queued for re-testing.
Comment #53
jibranRTBC as per #45.
Comment #54
larowlan+1 assuming bot agrees
Comment #55
alexpottlet's use $this->t() instead of t() now that this is there for us.
Comment #56
jibranFixed and back to RTBC.
Comment #57
alexpottCommitted 23fba80 and pushed to 8.x. Thanks!