Files: 
CommentFileSizeAuthor
#56 1997262-56.patch7.53 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,458 pass(es).
[ View ]
#56 interdiff.txt1.55 KBjibran
#53 1997262-53.patch7.53 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,171 pass(es).
[ View ]
#50 custom-block-1997262-50.patch7.5 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch custom-block-1997262-50.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#44 custom-block-1997262-44.patch7.54 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch custom-block-1997262-44.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#44 interdiff.txt1.4 KBtim.plunkett
#40 Screenshot from 2013-06-28 01:40:36.png68 KBtwistor
#38 interdiff.txt773 bytesandypost
#38 cbdf-1997262-37.patch6.14 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,972 pass(es).
[ View ]
#34 interdiff.txt1.18 KBandypost
#34 cbdf-1997262-34.patch6.25 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,076 pass(es).
[ View ]
#32 interdiff.txt4.46 KBandypost
#32 cbdf-1997262-32.patch6.19 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,087 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#30 custom-block-delete-form.30.patch5.67 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 56,660 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
#21 custom-block-delete-form.22.patch5.8 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch custom-block-delete-form.22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 1997262-block-confirm-form-13.interdiff.txt1017 bytesnick_schuch
#13 1997262-block-confirm-form-13.patch5.29 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 56,221 pass(es).
[ View ]
#9 1997262-block-confirm-form-9.interdiff.txt2.71 KBnick_schuch
#9 1997262-block-confirm-form-9.patch5.3 KBnick_schuch
FAILED: [[SimpleTest]]: [MySQL] 55,061 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#5 1997262-block-confirm-form-5.patch5.23 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 55,444 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#5 1997262-interdiff.txt2.8 KBkim.pepper
#1 1997262-convert-custom-block-delete-form.patch5.12 KBchertzog
FAILED: [[SimpleTest]]: [MySQL] 55,727 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new5.12 KB
FAILED: [[SimpleTest]]: [MySQL] 55,727 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new2.8 KB
new5.23 KB
FAILED: [[SimpleTest]]: [MySQL] 55,444 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

+++ 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.'

Status:Needs work» Needs review
StatusFileSize
new5.3 KB
FAILED: [[SimpleTest]]: [MySQL] 55,061 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.71 KB

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

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.

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

Status:Needs work» Needs review
StatusFileSize
new5.29 KB
PASSED: [[SimpleTest]]: [MySQL] 56,221 pass(es).
[ View ]
new1017 bytes

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

Status:Needs review» Reviewed & tested by the community

if its alright with the bot

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

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?

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?

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

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

Assigned:Unassigned» larowlan

re-rolling

Status:Needs work» Needs review
StatusFileSize
new5.8 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch custom-block-delete-form.22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll

Assigned:larowlan» Unassigned

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.

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.

Assigned:Unassigned» naxoc
Status:Needs review» Needs work

Status:Needs work» Needs review

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

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.

Status:Needs work» Needs review
StatusFileSize
new5.67 KB
FAILED: [[SimpleTest]]: [MySQL] 56,660 pass(es), 4 fail(s), and 1 exception(s).
[ View ]

Straight re-roll

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new6.19 KB
FAILED: [[SimpleTest]]: [MySQL] 58,087 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
new4.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.

Status:Needs work» Needs review
StatusFileSize
new6.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,076 pass(es).
[ View ]
new1.18 KB

For NG-entity. tests are green locally

No idea what those tags were for, we have tests

+++ 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

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

StatusFileSize
new6.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,972 pass(es).
[ View ]
new773 bytes

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

Status:Needs review» Reviewed & tested by the community

Looks good to me!

StatusFileSize
new68 KB

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

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.

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

twistors point is still valid though.

@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

Status:Needs work» Needs review
StatusFileSize
new1.4 KB
new7.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch custom-block-1997262-44.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Reviewed & tested by the community

I think it's good to go

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

Issue tags:+Needs reroll

needs reroll

Component:block.module» custom_block.module

Status:Needs work» Needs review
StatusFileSize
new7.5 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch custom-block-1997262-50.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Straight reroll.

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.

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll+FormInterface
StatusFileSize
new7.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,171 pass(es).
[ View ]

Straight reroll.

RTBC as per #45.

+1 assuming bot agrees

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.

Status:Needs work» Reviewed & tested by the community
Issue tags:+FormBase
StatusFileSize
new1.55 KB
new7.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,458 pass(es).
[ View ]

Fixed and back to RTBC.

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.