Files: 
CommentFileSizeAuthor
#28 1946454-system-admin-confirm-form-28.patch22.07 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,653 pass(es).
[ View ]
#23 1946454-system-admin-confirm-form-23.patch22.05 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 55,504 pass(es).
[ View ]
#23 interdiff.txt2.53 KBkim.pepper
#17 system-confirm-form-1946454-17.patch21.69 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,480 pass(es).
[ View ]
#17 interdiff.txt5.61 KBtim.plunkett
#15 interdiff.txt7.54 KBandypost
#15 1946454-system-admin-confirm-form-15.patch20.44 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,332 pass(es).
[ View ]
#13 1946454-system-admin-confirm-form-12.patch21.79 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 54,206 pass(es).
[ View ]
#13 interdiff.txt12.67 KBkim.pepper
#12 interdiff.txt3.51 KBandypost
#12 1946454-system-admin-confirm-form-12.patch21.67 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#10 interdiff.txt11.03 KBandypost
#10 1946454-system-admin-confirm-form-10.patch21.66 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,162 pass(es).
[ View ]
#7 interdiff.txt14.35 KBandypost
#7 1946454-system-admin-confirm-form-7.patch19.6 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 53,985 pass(es).
[ View ]
#4 1946454-system-admin-confirm-form-4.patch9.43 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 54,106 pass(es).
[ View ]
#4 interdiff.txt4.9 KBkim.pepper
#2 1946454-system-admin-confirm-form-2.patch5.29 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 53,878 pass(es).
[ View ]

Comments

Assigned:Unassigned» kim.pepper

Status:Active» Needs review
Issue tags:-WSCCI-conversion
StatusFileSize
new5.29 KB
PASSED: [[SimpleTest]]: [MySQL] 53,878 pass(es).
[ View ]

First go at converting the delete date format confirm form to form interface. This patch does not convert everything in the system.admin.inc.

Issue tags:+WSCCI-conversion

Tag was accidentally removed

StatusFileSize
new4.9 KB
new9.43 KB
PASSED: [[SimpleTest]]: [MySQL] 54,106 pass(es).
[ View ]

Added date_format_localize_reset form.

Assigned:kim.pepper» Unassigned

Not sure I know how to convert the remaining confirm_form code in system_modules and system_modules_uninstall. Looks like it needs to be refactored to separate some of the business logic from the form handling.

StatusFileSize
new19.6 KB
PASSED: [[SimpleTest]]: [MySQL] 53,985 pass(es).
[ View ]
new14.35 KB

Let's see what bot say.
This forms used as constructors so implementation have empty submitForm()

Also form code a bit refactored to use renderable array against theme('item_list')

Green! I think we need a follow-up here to clean-up this forms, seems some elements are not needed anymore

No, please NO out of scope changes. This is about converting it from procedural to OO, not changing all of the internals of the form. File a follow-up. Almost all of the interdiff from #7 should be reverted.

+++ b/core/modules/system/lib/Drupal/system/Form/DateFormatDeleteForm.phpundefined
@@ -17,19 +17,19 @@ class DateFormatDeleteForm extends ConfirmFormBase {
-   * Implements \Drupal\Core\Form\ConfirmFormBase::getQuestion().
+   * Implements ConfirmFormBase::getQuestion().
@@ -39,14 +39,14 @@ protected function getQuestion() {
-   * Implements \Drupal\Core\Form\ConfirmFormBase::getConfirmText().
+   * Overrides ConfirmFormBase::getConfirmText().
...
-   * Implements \Drupal\Core\Form\ConfirmFormBase::getCancelPath().
+   * Implements ConfirmFormBase::getCancelPath().
@@ -60,7 +60,7 @@ public function getFormID() {
-   * Implements \Drupal\Core\Form\FormInterface::buildForm().
+   * Overrides ConfirmFormBase::buildForm().
+++ b/core/modules/system/lib/Drupal/system/Form/DateFormatLocalizeResetForm.phpundefined
@@ -23,12 +24,12 @@ class DateFormatLocalizeResetForm extends ConfirmFormBase {
-   * Implements \Drupal\Core\Form\ConfirmFormBase::getQuestion().
+   * Implements ConfirmFormBase::getQuestion().
@@ -37,20 +38,27 @@ protected function getQuestion() {
   /**
-   * Implements \Drupal\Core\Form\ConfirmFormBase::getConfirmText().
+   * Overrides ConfirmFormBase::getConfirmText().
...
-   * Implements \Drupal\Core\Form\ConfirmFormBase::getCancelPath().
+   * Implements ConfirmFormBase::getCancelPath().
@@ -58,7 +66,7 @@ public function getFormID() {
-   * Implements \Drupal\Core\Form\FormInterface::buildForm().
+   * Overrides ConfirmFormBase::buildForm().
+++ b/core/modules/system/lib/Drupal/system/Form/ModulesInstallConfirmFrom.phpundefined
@@ -0,0 +1,88 @@
+   * Implements ConfirmFormBase::getQuestion().
...
+   * Overrides ConfirmFormBase::getConfirmText().
...
+   * Implements ConfirmFormBase::getCancelPath().
...
+   * Overrides ConfirmFormBase::getDescription().
...
+   * Overrides ConfirmFormBase::buildForm().
+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallConfirmFrom.phpundefined
@@ -0,0 +1,80 @@
+   * Implements ConfirmFormBase::getQuestion().
...
+   * Overrides ConfirmFormBase::getConfirmText().
...
+   * Implements ConfirmFormBase::getCancelPath().
...
+   * Overrides ConfirmFormBase::getDescription().
...
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Overrides ConfirmFormBase::buildForm().

All of these docblocks are wrong. Please switch them back, or to {@inheritdoc}

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallConfirmFrom.phpundefined
@@ -0,0 +1,80 @@
+      $form['uninstall'][$module] = array('#type' => 'hidden',
+        '#value' => 1,

This is bizarre that its only half split across lines.

+++ b/core/modules/system/system.admin.incundefined
@@ -1273,8 +1229,9 @@ function system_modules_uninstall($form, $form_state = NULL) {
-  if (!empty($form_state['storage']) && $confirm_form = system_modules_uninstall_confirm_form($form_state['storage'])) {
-    return $confirm_form;
+  if (!empty($form_state['storage']['uninstall']) && $modules = array_filter($form_state['storage']['uninstall'])) {
+    $confirm_form = new ModulesUninstallConfirmFrom();
+    return $confirm_form->buildForm($form, $form_state, $modules);

This should redirect to the path, I believe. We shouldn't be calling it directly here at all.

StatusFileSize
new21.66 KB
PASSED: [[SimpleTest]]: [MySQL] 54,162 pass(es).
[ View ]
new11.03 KB

Reverted namespaces in comments
Render preserved as is
Filed follow-up #1964044: Change module list in install/uninstall confirm form to render array

Status:Needs review» Needs work

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesInstallConfirmFrom.phpundefined
@@ -0,0 +1,90 @@
+class ModulesInstallConfirmFrom extends ConfirmFormBase {

Should be ModulesInstallConfirmForm (not From)

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallConfirmFrom.phpundefined
@@ -0,0 +1,79 @@
+class ModulesUninstallConfirmFrom extends ConfirmFormBase {

Should be ModulesUninstallConfirmForm

Status:Needs work» Needs review
StatusFileSize
new21.67 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new3.51 KB

Thanx!
The only questionable part a comment - I need help to properly figure out this edge case

+++ b/core/modules/system/system.admin.incundefined
@@ -1258,23 +1212,24 @@ function system_modules_submit($form, &$form_state) {
+    // Contents of confirm form is injected here because already in form
+    // building function.
+    $confirm_form = new ModulesUninstallConfirmFrom();
+    return $confirm_form->buildForm($form, $form_state, $modules);

Is this comment enough to explain inline usage of public form method?

StatusFileSize
new12.67 KB
new21.79 KB
PASSED: [[SimpleTest]]: [MySQL] 54,206 pass(es).
[ View ]

Fixes typos in #11

Doh! Too quick.

StatusFileSize
new20.44 KB
PASSED: [[SimpleTest]]: [MySQL] 54,332 pass(es).
[ View ]
new7.54 KB

Re-roll for #1906474: [policy adopted] Stop documenting methods with "Overrides …"

+++ b/core/modules/system/system.admin.incundefined
@@ -7,10 +7,11 @@
use Drupal\Core\Ajax\ReplaceCommand;
-use Symfony\Component\HttpFoundation\JsonResponse;

This out of scope

Status:Needs review» Needs work

Besides the below, this looks great

+++ b/core/modules/system/lib/Drupal/system/Form/DateFormatLocalizeResetForm.phpundefined
@@ -0,0 +1,87 @@
+    config('locale.config.' . $this->language->langcode . '.system.date')->delete();

lets inject config.factory and use it instead

Status:Needs work» Needs review
StatusFileSize
new5.61 KB
new21.69 KB
PASSED: [[SimpleTest]]: [MySQL] 55,480 pass(es).
[ View ]

Okay, cleaned up a bit.

Status:Needs review» Reviewed & tested by the community

This is looking really great so far, so here is just a tiny little question which is independent from the actual review.

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesInstallConfirmForm.phpundefined
@@ -0,0 +1,90 @@
+  public function buildForm(array $form, array &$form_state, $modules = array(), $storage = array()) {

In general I'm wondering how we could/should document this additional parameters on this kind of methods.

The phpdoc manual suggests you can put extra @params after the {@inheritdoc} and they will be kept. http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutoria...

It's great that you actually linked the reference!

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/system/lib/Drupal/system/Form/DateFormatDeleteForm.phpundefined
@@ -0,0 +1,109 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function buildForm(array $form, array &$form_state, $format_id = NULL) {
+++ b/core/modules/system/lib/Drupal/system/Form/DateFormatLocalizeResetForm.phpundefined
@@ -0,0 +1,105 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function buildForm(array $form, array &$form_state, $langcode = NULL) {
+++ b/core/modules/system/lib/Drupal/system/Form/ModulesInstallConfirmForm.phpundefined
@@ -0,0 +1,90 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function buildForm(array $form, array &$form_state, $modules = array(), $storage = array()) {
+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallConfirmForm.phpundefined
@@ -0,0 +1,80 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function buildForm(array $form, array &$form_state, $modules = array()) {

Need extra params documenting... as per #19

Related issue #1852454: Restrict module and theme name lengths to 50 characters should make changes in module list so I filed #1990544: Convert system_modules() to a Controller

Probably better to convert system_modules() to controller first

Status:Needs work» Needs review
StatusFileSize
new2.53 KB
new22.05 KB
PASSED: [[SimpleTest]]: [MySQL] 55,504 pass(es).
[ View ]

Re-rolled against head and updated docs as in #22.

Status:Needs review» Reviewed & tested by the community

great!

I'm wondering whether something like a naming scheme exists for the route names.
It seems like the router names will be totally inconsistent, if we don't settle with one standard.
In, system.routing.yml, there are already the router names "system.cron" and "system.machine_name_transliterate".
Now you're using "date_format_delete" and "date_format_localize_reset" here.
And in #1990544: Convert system_modules() to a Controller, I'm using "system_modules_list" and "system_modules_list_confirm".
Either of these schemes should be fine, but probably not a mixture of three different schemes.

Also, I'm not sure it makes sense to pull apart system_modules() and its confirm form in two issues.
After either of the patches lands, we need to significantly reroll the other. We can do that though.

[edit:] same for #1993202: Convert system_modules_uninstall() to a Controller

@pancho could you open a separate issue for the naming scheme? I think we should sort that out because they're obviously inconsistent now, but it's going to need a dedicated discussion for it.

+++ b/core/modules/system/lib/Drupal/system/Form/DateFormatDeleteForm.phpundefined
@@ -0,0 +1,111 @@
+   * @param string $format_id

I think we need a newline before this @param statement

+++ b/core/modules/system/lib/Drupal/system/Form/DateFormatLocalizeResetForm.phpundefined
@@ -0,0 +1,108 @@
+   * @param string $langcode

Same

+++ b/core/modules/system/lib/Drupal/system/Form/ModulesInstallConfirmForm.phpundefined
@@ -0,0 +1,94 @@
+   * @param array $modules
+++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallConfirmForm.phpundefined
@@ -0,0 +1,82 @@
+   * @param array $modules

Same =)

Shouldn't stop a rtbc but maybe somebody is bored and wants to reroll :p

StatusFileSize
new22.07 KB
PASSED: [[SimpleTest]]: [MySQL] 55,653 pass(es).
[ View ]

Disclaimer: i was not bored:)
it just needed a reroll, so i fixed #27

Status:Fixed» Reviewed & tested by the community

#26:
You're right and I'm gonna do that.
Actually the route_names are not that important. We can easily streamline them at a later point.
More important is a consistent naming scheme for the classes (= their filenames). We don't want to move around classes and files later, if avoidable. Didn't find anything relevant about that.

Regarding the route names, it however seems that a naming scheme has become prevalent that includes the module name.
We're having 'system_file_system_settings' or 'system_site_maintenance_mode', so I'd rather use 'system_date_format_delete' and 'system_date_format_localize_reset' here.

Don't let my comments unduly delay this patch though.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

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