Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kim.pepper’s picture

Assigned: Unassigned » kim.pepper
kim.pepper’s picture

Status: Active » Needs review
Issue tags: -WSCCI-conversion
FileSize
5.29 KB

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

kim.pepper’s picture

Issue tags: +WSCCI-conversion

Tag was accidentally removed

kim.pepper’s picture

Added date_format_localize_reset form.

kim.pepper’s picture

Assigned: kim.pepper » Unassigned
kim.pepper’s picture

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.

andypost’s picture

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

andypost’s picture

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

tim.plunkett’s picture

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.

andypost’s picture

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

kim.pepper’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
21.67 KB
3.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?

kim.pepper’s picture

Fixes typos in #11

kim.pepper’s picture

Doh! Too quick.

andypost’s picture

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

ParisLiakos’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.61 KB
21.69 KB

Okay, cleaned up a bit.

dawehner’s picture

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.

kim.pepper’s picture

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

dawehner’s picture

It's great that you actually linked the reference!

alexpott’s picture

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

andypost’s picture

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
2.53 KB
22.05 KB

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

andypost’s picture

Status: Needs review » Reviewed & tested by the community

great!

Pancho’s picture

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

catch’s picture

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

aspilicious’s picture

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

ParisLiakos’s picture

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

Pancho’s picture

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.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

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