Problem/Motivation

Right now, ConfirmFormBase is needed for a confirmation form.
There is no way to provide something like a delete form using EntityFormControllerInterface

Proposed resolution

Add EntityConfirmFormBase

Remaining tasks

User interface changes

n/a

API changes

API addition

Files: 
CommentFileSizeAuthor
#37 confirm_alter-2011018-37.patch1.77 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,752 pass(es).
[ View ]
#36 confirm_alter-2011018-36.patch1.77 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#35 confirm_alter-2011018-35.patch1.21 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,832 pass(es).
[ View ]
#31 entity-confirm-2011018-31.patch115.32 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,054 pass(es).
[ View ]
#31 interdiff.txt989 bytestim.plunkett
#26 entity-confirm-form-2011018-26.patch115.32 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,694 pass(es).
[ View ]
#25 entity-confirm-form-2011018-25.patch114.86 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,676 pass(es).
[ View ]
#21 entity-confirm-form-2011018-21.patch114.88 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,799 pass(es).
[ View ]
#21 interdiff.txt4.11 KBtim.plunkett
#18 entity-confirm-form-2011018-18.patch110.78 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,324 pass(es), 26 fail(s), and 16 exception(s).
[ View ]
#18 interdiff.txt2.39 KBtim.plunkett
#15 entity-confirm-form-2011018-15.patch110.64 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,958 pass(es).
[ View ]
#15 interdiff.txt3.29 KBtim.plunkett
#13 entity-confirm-form-2011018-13.patch109.44 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,530 pass(es), 23 fail(s), and 13 exception(s).
[ View ]
#13 interdiff.txt29.62 KBtim.plunkett
#11 entity-confirm-form-2011018-11.patch92.21 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 54,325 pass(es), 664 fail(s), and 146 exception(s).
[ View ]
#11 interdiff.txt21.65 KBtim.plunkett
#8 entity-confirm-form-2011018-8.patch76.42 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-confirm-form-2011018-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 interdiff.txt44.53 KBtim.plunkett
#6 entity-confirm-form-2011018-6.patch39.18 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 54,563 pass(es), 821 fail(s), and 255 exception(s).
[ View ]
#6 interdiff.txt409 bytestim.plunkett
#4 entity-confirm-form-2011018-4.patch39.1 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 54,506 pass(es), 826 fail(s), and 255 exception(s).
[ View ]
#4 interdiff.txt2.4 KBtim.plunkett
#2 entity-confirm-form-2011018-2.patch38.45 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,265 pass(es).
[ View ]
#2 interdiff.txt35.09 KBtim.plunkett
#1 entity-confirm-form-2011018-1.patch7.72 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,409 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new7.72 KB
PASSED: [[SimpleTest]]: [MySQL] 55,409 pass(es).
[ View ]

We have no interface called ConfirmFormInterface, since everything unique to ConfirmFormBase is protected.
So these classes are not related in any technical way.

One solution is to make the methods public. They all return strings. It's not really important if these are called or not.

Leaving out that switch to keep it reviewable.

This adds the new base class and does one conversion (View delete form).

StatusFileSize
new35.09 KB
new38.45 KB
PASSED: [[SimpleTest]]: [MySQL] 55,265 pass(es).
[ View ]

Here it is with the interface.

overall i like this:) one more DX win!

+++ b/core/lib/Drupal/Core/Entity/EntityConfirmFormBase.phpundefined
@@ -0,0 +1,99 @@
+    if (isset($_GET['destination'])) {
+      $options = drupal_parse_url($_GET['destination']);

Since we are here..
If we are depending on Request why dont we have it as argument on buildForm() and store it in a property?
Like we did for ban module..

Unless of course its not possible for confirm forms yet?

StatusFileSize
new2.4 KB
new39.1 KB
FAILED: [[SimpleTest]]: [MySQL] 54,506 pass(es), 826 fail(s), and 255 exception(s).
[ View ]

Sure why not!

StatusFileSize
new409 bytes
new39.18 KB
FAILED: [[SimpleTest]]: [MySQL] 54,563 pass(es), 821 fail(s), and 255 exception(s).
[ View ]

Missed a use statement.

Status:Needs review» Needs work
StatusFileSize
new44.53 KB
new76.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-confirm-form-2011018-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Well that would mean reimplementing all of the forms. That can be a follow-up and we can commit the patch in #2, or we could do it all here.

This is a start, there are still a half dozen forms left.

Status:Needs work» Needs review

Issue tags:+FormInterface
StatusFileSize
new21.65 KB
new92.21 KB
FAILED: [[SimpleTest]]: [MySQL] 54,325 pass(es), 664 fail(s), and 146 exception(s).
[ View ]

Last couple.

Because of inheritance, I had to add both EntityConfirmFormBase and EntityNGConfirmFormBase, so the diffstat is a bit lopsided. It won't be that bad in the end.

StatusFileSize
new29.62 KB
new109.44 KB
FAILED: [[SimpleTest]]: [MySQL] 55,530 pass(es), 23 fail(s), and 13 exception(s).
[ View ]

I missed the ones not called "Delete" :)

StatusFileSize
new3.29 KB
new110.64 KB
PASSED: [[SimpleTest]]: [MySQL] 56,958 pass(es).
[ View ]

Alright, fixed those last test failures.

+++ b/core/lib/Drupal/Core/Entity/EntityConfirmFormBase.phpundefined
@@ -0,0 +1,108 @@
+ * Provides an generic base class for an entity-based confirmation form.

my english are not perfect but i think its: *a* generic

+++ b/core/modules/menu/menu.moduleundefined
@@ -142,14 +142,18 @@ function menu_menu() {
-function menu_entity_info_alter(&$entity_info) {
+function menu_entity_info(&$entity_info) {

hmm i can see why you do this, but maybe there is a reason it was like that?

+++ b/core/modules/node/node.moduleundefined
@@ -1930,6 +1930,10 @@ function theme_node_recent_content($variables) {
+  if ($form_state['controller']->getOperation() == 'delete') {
+    return;
+  }

this could use a comment

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Form/BreakLockForm.phpundefined
@@ -7,18 +7,17 @@
+use Drupal\Core\Entity\EntityConfirmFormBase;
+use Drupal\Core\Entity\EntityControllerInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;
-
-use Drupal\Core\ControllerInterface;
-use Drupal\Core\Form\ConfirmFormBase;
-use Drupal\views\ViewStorageInterface;
use Drupal\Core\Entity\EntityManager;
use Drupal\user\TempStoreFactory;
+use Symfony\Component\HttpFoundation\Request;

since we are touching those lets order them alphabetically

StatusFileSize
new2.39 KB
new110.78 KB
FAILED: [[SimpleTest]]: [MySQL] 57,324 pass(es), 26 fail(s), and 16 exception(s).
[ View ]

hook_entity_info_alter() is used to override other set values, hook_entity_info() is used to provide new values for other entity types.
Views UI does this for Views.

Fixed the other 3 things.

Status:Needs review» Reviewed & tested by the community

great..only minus here is that we are duplicating logic between entity and entityNG forms, but one is to be axed anyway..so marking rtbc, i am sure bot will have no problems

great job, thanks!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, entity-confirm-form-2011018-18.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.11 KB
new114.88 KB
PASSED: [[SimpleTest]]: [MySQL] 57,799 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community
Issue tags:+DX (Developer Experience)

the interdiff shows how many less lines you have to write now for an entity delete confirm form:)

Status:Reviewed & tested by the community» Needs review

I don't think we should change the methods from protected to public. Why not an empty interface if necessary? What's the problem with just having the protected method on the abstract class - that's what abstract classes are good at - enforcing internal implementation rather than public API.

Status:Needs review» Reviewed & tested by the community

The patch in #1 does what I think you're asking.

Except then you end up with two (three counting NG) base classes, ConfirmFormBase and EntityConfirmFormBase with the same protected methods and the same docs and the same functionality, but with nothing shared between.

Nothing needs to check the interface, so an empty one would be useless.

I discussed this issue at length with msonnabaum, and he agreed making them public is the best we can do without multiple inheritance.

Tentatively re-RTBC-ing.

StatusFileSize
new114.86 KB
PASSED: [[SimpleTest]]: [MySQL] 57,676 pass(es).
[ View ]

StatusFileSize
new115.32 KB
PASSED: [[SimpleTest]]: [MySQL] 57,694 pass(es).
[ View ]

Status:Reviewed & tested by the community» Needs work
Issue tags:-DX (Developer Experience), -FormInterface

The last submitted patch, entity-confirm-form-2011018-26.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+DX (Developer Experience), +FormInterface

#26: entity-confirm-form-2011018-26.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

Couple of minor nits

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,4 +14,11 @@
+  /**
+   * Removes all items from a feed.
+   *
+   * @return self
+   */
+  public function removeItems();

The implement does not return $this and doing so would be unnecessary.

+++ b/core/modules/image/lib/Drupal/image/Form/ImageStyleDeleteForm.phpundefined
@@ -7,64 +7,46 @@
+  public function form(array $form, array &$form_state) {
+    $replacement_styles = array_diff_key(image_style_options(), array($this->entity->id() => ''));
     $form['replacement'] = array(
       '#title' => t('Replacement style'),
       '#type' => 'select',
@@ -72,16 +54,16 @@ public function buildForm(array $form, array &$form_state, ImageStyle $image_sty
@@ -72,16 +54,16 @@ public function buildForm(array $form, array &$form_state, ImageStyle $image_sty
       '#empty_option' => t('No replacement, just delete'),
     );
-    return parent::buildForm($form, $form_state);
+    return $form;

Talked about this with @timplunkett and he said we should be calling parent::form() here... probably before we add $form['replacement'].

Status:Needs work» Needs review
StatusFileSize
new989 bytes
new115.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,054 pass(es).
[ View ]

Nice catch on the @return self.

We do want to call parent::form(), but still at the end. It adds in default values for stuff like langcode, and we've been calling it afterward in other places.

Status:Needs review» Reviewed & tested by the community

These chances are fine!

Title:Reconcile entity forms and confirm formsChange notice: Reconcile entity forms and confirm forms
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed 73fbcf8 and pushed to 8.x. Thanks!

Need to review existing change records to see if any need an update.

Status:Active» Needs review
StatusFileSize
new1.21 KB
PASSED: [[SimpleTest]]: [MySQL] 57,832 pass(es).
[ View ]

What about this, to ensure they're not caught by BASE_FORM_ID_alter?

StatusFileSize
new1.77 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

I think better make a name '_confirm_form' suffix.
It works fine now but there's no ability to pass anything from container to form

StatusFileSize
new1.77 KB
PASSED: [[SimpleTest]]: [MySQL] 57,752 pass(es).
[ View ]

Removed useless spaces.
Change notice should point about that alter should use hook_form_$entitytype_confirm_form_alter

Status:Needs review» Reviewed & tested by the community

Thanks @andypost, sorry for the temporary regression.

Status:Reviewed & tested by the community» Active

Committed fd5f765 and pushed to 8.x. Thanks!

Assigned:tim.plunkett» Unassigned

Status:Active» Needs review

As per confirm_form() deprecated in favor of \Drupal\Core\Form\ConfirmFormBase form controller should extend Drupal\Core\Form\ConfirmFormBase.
If confirm form is used to provide some kind of entity function then form controller should extend Drupal\Core\Entity\EntityConfirmFormBase see class VocabularyDeleteForm for further details.
For entity confirm form routing.yml entry should look like this

taxonomy_vocabulary_delete:
  pattern: '/admin/structure/taxonomy/manage/{taxonomy_vocabulary}/delete'
  defaults:
    _entity_form: 'taxonomy_vocabulary.delete'
  requirements:
    _entity_access: 'taxonomy_vocabulary.delete'
see taxonomy.routing.yml for further details.
As this is entity form _entity_form: 'taxonomy_vocabulary.delete' we have to add form class Drupal\taxonomy\Form\VocabularyDeleteForm to entity info using @EntityType or hook_entity_info or hook_entity_info_alter.
See Core/Entity/Vocabulary.php "delete" = "Drupal\taxonomy\Form\VocabularyDeleteForm" for @EntityType entry or action_entity_info $entity_info['action']['controllers']['form']['delete'] = 'Drupal\action\Form\ActionDeleteForm'; for hook_entity_info_alter entry.

Issue summary:View changes
Issue tags:+Missing change record

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

The patch for this issue was committed June 17, 2013, meaning that the change record updates for this issue have been outstanding for over seven months. Someone knowledgeable about the issue (cough @tim.plunkett) should review the draft @jibran has provided and ensure that the relevant change records get updated.

Status:Needs review» Needs work

This issue needs work, either because of jibran's comment: https://drupal.org/comment/7683993#comment-7683993, if not that, then the change notice should be written. So changing status accordingly.

Title:Change notice: Reconcile entity forms and confirm formsReconcile entity forms and confirm forms
Status:Needs work» Fixed
Issue tags:-Needs change record, -Missing change record

I added an example for EntityConfirmFormBase in https://drupal.org/node/1945416
since the regression with form ids is fixed, i dont think there is something more needed here

@balagan it was NR because in #41 I purposed the change record draft. Issue is was fixed a long ago.

Status:Fixed» Closed (fixed)

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