Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chertzog’s picture

Status: Active » Needs review
FileSize
5.12 KB

YAY! my first non docs Drupal 8 core patch that actually does something!

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -98,14 +98,7 @@ function custom_block_menu() {
-    'weight' => 1,
-    'type' => MENU_LOCAL_TASK,
-    'context' => MENU_CONTEXT_INLINE,

This will remove the local task. I guess there isn't a button now?

+++ b/core/modules/block/custom_block/custom_block.pages.incundefined
@@ -80,44 +80,3 @@ function custom_block_edit(CustomBlock $block) {
-  // @todo Delete all configured instances of the block.

Please don't remove the todo

+++ b/core/modules/block/custom_block/custom_block.routing.ymlundefined
@@ -12,3 +12,10 @@ custom_block_add_page:
+  requirements:
+    _permission: 'administer blocks'

Hmm your requirement check doesn't match the original entity_access delete check.

Not sure how that works with the new routing api.

chertzog’s picture

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

aspilicious’s picture

1) 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 :).

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
2.8 KB
5.23 KB

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

Status: Needs review » Needs work

The last submitted patch, 1997262-block-confirm-form-5.patch, failed testing.

larowlan’s picture

larowlan’s picture

+++ b/core/modules/block/custom_block/custom_block.pages.incundefined
@@ -80,44 +80,3 @@ function custom_block_edit(CustomBlock $block) {
-  watchdog('custom_block', 'Custom block %label has been deleted.', array('%label' => $block->label()), WATCHDOG_NOTICE);

This wasn't added back in the new Form

+++ b/core/modules/block/custom_block/custom_block.routing.ymlundefined
@@ -12,3 +12,13 @@ custom_block_add_page:
+    converters:

Not needed, no conversion

+++ b/core/modules/block/custom_block/custom_block.routing.ymlundefined
@@ -12,3 +12,13 @@ custom_block_add_page:
+    _entity_access: 'custom_block.view'

custom_block.delete?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Form/CustomBlockDeleteForm.phpundefined
@@ -0,0 +1,74 @@
+ * Provides a deletion confirmation form for the block instance deletion form.

This isn't an instance, just a content entity

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Form/CustomBlockDeleteForm.phpundefined
@@ -0,0 +1,74 @@
+    return t('Are you sure you want to delete the block %name?', array('%name' => $this->block->label()));

Existing text was "Are you sure you want to delete %name", hence the failing test.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Form/CustomBlockDeleteForm.phpundefined
@@ -0,0 +1,74 @@
+   * @param \Drupal\custom_block\Plugin\Core\Entity\CustomBlock $block

$custom_block, not $block

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Form/CustomBlockDeleteForm.phpundefined
@@ -0,0 +1,74 @@
+   *   The block instance.

The custom block entity

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Form/CustomBlockDeleteForm.phpundefined
@@ -0,0 +1,74 @@
+    drupal_set_message(t('The block %name has been removed.', array('%name' => $this->block->label())));

Original message was 'Custom block %label has been deleted.'

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
5.3 KB
2.71 KB

Here is a patch with the fixes identified in #8.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Assuming bot agrees

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1997262-block-confirm-form-9.patch, failed testing.

larowlan’s picture

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Form/CustomBlockDeleteForm.phpundefined
@@ -0,0 +1,75 @@
+ * Provides a deletion confirmation form for the block content entity form.

Provides a confirmation form for deleting a custom block entity.

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
5.29 KB
1017 bytes

Dam "?"! Here we go and the grammar update to.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

if its alright with the bot

larowlan’s picture

This will collide with #1994146: WSOD after deleting a custom block content entity, that is critical so should go in first.

andypost’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests, +Create issues for missing UnitTests add above

Let's get #1994146: WSOD after deleting a custom block content entity in first and then add test

+++ b/core/modules/block/custom_block/custom_block.pages.incundefined
@@ -80,44 +80,3 @@ function custom_block_edit(CustomBlock $block) {
-  // @todo Delete all configured instances of the block.
...
-  $block->delete();

Is there really deletion of instances happen? unit test of the form?

+++ b/core/modules/block/custom_block/custom_block.routing.ymlundefined
@@ -12,3 +12,13 @@ custom_block_add_page:
+  pattern: '/block/{custom_block}/delete'
+  options:
+    converters:
+      custom_block: 'custom_block'

Any reason for converters?

nick_schuch’s picture

1) 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?

andypost’s picture

Yep, converters are gone but:

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Form/CustomBlockDeleteForm.phpundefined
@@ -0,0 +1,75 @@
+    // @todo Delete all configured instances of the block.
+    $this->block->delete();

I mean this @todo is not needed if deletion of instances would have a test

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Form/CustomBlockDeleteForm.phpundefined
@@ -0,0 +1,75 @@
+    // @todo Delete all configured instances of the block.
+    $this->block->delete();

Needs re-roll after #1994146: WSOD after deleting a custom block content entity

larowlan’s picture

Assigned: Unassigned » larowlan

re-rolling

larowlan’s picture

Status: Needs work » Needs review
FileSize
5.8 KB

Re-roll

larowlan’s picture

Assigned: larowlan » Unassigned
andypost’s picture

Status: Needs review » Reviewed & tested by the community

RTBC if bot green

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests, -Create issues for missing UnitTests add above

The last submitted patch, custom-block-delete-form.22.patch, failed testing.

nick_schuch’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +Create issues for missing UnitTests add above

[MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].

#21: custom-block-delete-form.22.patch queued for re-testing.

naxoc’s picture

Assigned: Unassigned » naxoc
Status: Needs review » Needs work
nick_schuch’s picture

Status: Needs work » Needs review

#21: custom-block-delete-form.22.patch queued for re-testing.

nick_schuch’s picture

If comes back green will RTBC as per #23.

Status: Needs review » Needs work

The last submitted patch, custom-block-delete-form.22.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
5.67 KB

Straight re-roll

Status: Needs review » Needs work

The last submitted patch, custom-block-delete-form.30.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
6.19 KB
4.46 KB

Should be build on top of EntityConfirmFormBase also not sure that form name should be defined

Status: Needs review » Needs work

The last submitted patch, cbdf-1997262-32.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
6.25 KB
1.18 KB

For NG-entity. tests are green locally

larowlan’s picture

Issue tags: -Needs tests, -Create issues for missing UnitTests add above

No idea what those tags were for, we have tests

tim.plunkett’s picture

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Form/CustomBlockDeleteForm.phpundefined
@@ -0,0 +1,72 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getFormID() {
+    return 'custom_block_delete_form';
+  }

No reason to override the one coming from EntityConfirmFormBase

Otherwise it's RTBC

twistor’s picture

So the edit form is showing up on the delete page.

andypost’s picture

FileSize
6.14 KB
773 bytes

Agreed, if there's no tests that alters the form

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

twistor’s picture

This is what I see when I try to delete a custom block with this patch.

naxoc’s picture

Status: Reviewed & tested by the community » Needs work

This needs a re-roll.

Like twistor I am also confused about the form. That does not look right.

naxoc’s picture

Woops. Sorry. Does not need a reroll - but I need glasses.

twistors point is still valid though.

andypost’s picture

@twistor please provide a steps to reproduce, I can't reproduce this

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Form/CustomBlockDeleteForm.phpundefined
@@ -0,0 +1,65 @@
+class CustomBlockDeleteForm extends EntityNGConfirmFormBase {

Looks like it different bug of this class.
Not sure why fields are displayed here

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
7.54 KB

I've solved that before, I'm moving the fix up a level.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

I think it's good to go

xjm’s picture

#44: custom-block-1997262-44.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, custom-block-1997262-44.patch, failed testing.

andypost’s picture

Issue tags: +Needs reroll

needs reroll

tim.plunkett’s picture

Component: block.module » custom_block.module
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.5 KB

Straight reroll.

jibran’s picture

Issue tags: -Needs reroll

#50: custom-block-1997262-50.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs reroll

The last submitted patch, custom-block-1997262-50.patch, failed testing.

jibran’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll +FormInterface
FileSize
7.53 KB

Straight reroll.

RTBC as per #45.

larowlan’s picture

+1 assuming bot agrees

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Form/CustomBlockDeleteForm.phpundefined
@@ -0,0 +1,65 @@
+class CustomBlockDeleteForm extends EntityNGConfirmFormBase {
...
+    return t('Are you sure you want to delete %name?', array('%name' => $this->entity->label()));
...
+    return t('Delete');
...
+    drupal_set_message(t('Custom block %label has been deleted.', array('%label' => $this->entity->label())));

let's use $this->t() instead of t() now that this is there for us.

jibran’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +FormBase
FileSize
1.55 KB
7.53 KB

Fixed and back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 23fba80 and pushed to 8.x. Thanks!

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