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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
7.72 KB

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

tim.plunkett’s picture

Here it is with the interface.

ParisLiakos’s picture

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?

tim.plunkett’s picture

Sure why not!

tim.plunkett’s picture

Missed a use statement.

tim.plunkett’s picture

Status: Needs review » Needs work
FileSize
44.53 KB
76.42 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Issue tags: +FormInterface
FileSize
21.65 KB
92.21 KB

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.

tim.plunkett’s picture

I missed the ones not called "Delete" :)

tim.plunkett’s picture

Alright, fixed those last test failures.

ParisLiakos’s picture

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

ParisLiakos’s picture

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

tim.plunkett’s picture

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.

ParisLiakos’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.11 KB
114.88 KB
ParisLiakos’s picture

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

catch’s picture

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

tim.plunkett’s picture

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.

tim.plunkett’s picture

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

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
989 bytes
115.32 KB

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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

These chances are fine!

alexpott’s picture

Title: Reconcile entity forms and confirm forms » Change 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.

andypost’s picture

tim.plunkett’s picture

Status: Active » Needs review
FileSize
1.21 KB

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

andypost’s picture

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

andypost’s picture

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

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @andypost, sorry for the temporary regression.

alexpott’s picture

Status: Reviewed & tested by the community » Active

Committed fd5f765 and pushed to 8.x. Thanks!

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
jibran’s picture

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.

catch’s picture

xjm’s picture

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.

balagan’s picture

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.

ParisLiakos’s picture

Title: Change notice: Reconcile entity forms and confirm forms » Reconcile 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

jibran’s picture

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