Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mhagedon’s picture

Getting started on this shortly...

mhagedon’s picture

mhagedon’s picture

Here's a partial patch. It'll fail testing, but on top of that the functional tests are blocked by #1952394: Add configuration translation user interface module in core. Next step is probably to figure out how best to populate the Language object in buildForm. Not marking "needs review" since it will fail.

Gábor Hojtsy’s picture

Status: Active » Needs review
Issue tags: +D8MI, +language-base
Gábor Hojtsy’s picture

Status: Needs review » Needs work

The last submitted patch, language-wscci-conversion-2003592-3.patch, failed testing.

Pancho’s picture

Assigned: mhagedon » Pancho

Rather this one partly depends on #2005778: Convert language_admin_overview_form to a Controller.
Also, this one and #2038291: Convert language_admin_add_form to a Controller are heavily interdepending, so we need to refactor the logic from composition to inheritance. Well, we don't necessarily have to - both have their merits even in OOP -, but it would make the code cleaner plus improve the experience with JS being disabled.

So I started a patch that should be CNR by tomorrow, if I find time finishing it. Hope it's okay with @mhagedon if I reassign this to me until Sunday or so.

penyaskito’s picture

#2005778: Convert language_admin_overview_form to a Controller landed, including LanguageAccessController.

webflo’s picture

Status: Needs work » Needs review
FileSize
9.2 KB

This patch adds a new LanguageFormController. Maybe we should postpone #2038291: Convert language_admin_add_form to a Controller?

Status: Needs review » Needs work

The last submitted patch, language-edit-form-controller-2003592-9.patch, failed testing.

Pancho’s picture

Too bad, this seriously has been double work. :(
It's not been without reason that I said I'm assigning to this until Sunday.

Gábor Hojtsy’s picture

Issue tags: +API change, +API clean-up

Add API change tags.

Pancho’s picture

Ouch, finishing my patch is overdue. I'm sorry, it will be my next task for today.

Postponed #2038291: Convert language_admin_add_form to a Controller on this one here.

Gábor Hojtsy’s picture

@Pancho, @webflo, still working on this one?

andypost’s picture

Title: Convert language_admin_edit_form to a Controller » Convert language_admin_add_form and language_admin_edit_form to a Controllers

Closed as duplicate #2038291: Convert language_admin_add_form to a Controller

Both forms have common part so needs base class like image styles uses.
Also I'm not sure that there's api change because _language_admin_common_controls() is not API function so should be moved to the base class.

Suppose the names should be:
Form/LanguageAddForm + Form/LanguageEditForm inherited from abstract Form/LanguageFormBase with corresponding inhected services by implementing EntityControllerInterface

And require #2031277-11: Implement checkCreateAccess on LanguageAccessController

andypost’s picture

Status: Needs work » Needs review
FileSize
24.28 KB

Suppose approach should be different, initial patch

Status: Needs review » Needs work

The last submitted patch, 2038291-lang-add-16.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
24.52 KB
3.36 KB

Another attempt, in chrome html5 validation still disallow submit predefined language

Status: Needs review » Needs work

The last submitted patch, 2038291-lang-add-18.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
24.49 KB
953 bytes

Fix some test failures, still can't fix #states for predefined language in chrome

Status: Needs review » Needs work

The last submitted patch, 2038291-lang-add-20.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
24.56 KB
7.04 KB

Removed standard actions
Renamed validation
a bit of clean-up

andypost’s picture

Issue tags: +JavaScript

Tagging because hidden #state does not allows to submit Add form by mouse, but "space-button" works.
Also this works before conversion

PS: chrome 30.0.1599.10 dev

tim.plunkett’s picture

  1. +++ b/core/modules/language/lib/Drupal/language/Form/LanguageAddForm.php
    @@ -0,0 +1,141 @@
    +    parent::commonForm($form['custom_language']);
    
    +++ b/core/modules/language/lib/Drupal/language/Form/LanguageEditForm.php
    @@ -0,0 +1,60 @@
    +    parent::commonForm($form);
    

    This is not a proper use of parent::, it should be something like

    $form = $this->addCommonFormElements($form, $form_state);

  2. +++ b/core/modules/language/lib/Drupal/language/Form/LanguageEditForm.php
    @@ -0,0 +1,60 @@
    +    $actions['submit'] = array(
    +      '#type' => 'submit',
    +      '#value' => $this->translator->translate('Save language'),
    +      '#validate' => array(array($this, 'validateCommon')),
    +      '#submit' => array(array($this, 'submitForm')),
    +    );
    

    This doesn't make much sense to me.

  3. +++ b/core/modules/language/lib/Drupal/language/Form/LanguageFormBase.php
    @@ -0,0 +1,114 @@
    +   */
    +  protected $translator;
    

    This should have used t() until FormBase was in.

  4. +++ b/core/modules/language/lib/Drupal/language/Form/LanguageFormBase.php
    @@ -0,0 +1,114 @@
    +  public function validateCommon(array $form, array &$form_state) {
    

    Why isn't this just validateForm()?

andypost’s picture

FileSize
15.16 KB
12.08 KB

Fixed translator

dawehner’s picture

Status: Needs review » Needs work

t() could be just replaced by $this->t() in this patch.

+++ b/core/modules/language/lib/Drupal/language/Form/LanguageAddForm.php
@@ -0,0 +1,144 @@
+    // @todo Remove this when https://drupal.org/node/1981644 is in.
+    drupal_set_title(t('Add language'));

Let's set the #title on the render array.

andypost’s picture

There's a conceptual question here - old code and currently used functions operates on Language object not the language entity.
So should we build the forms on top of the language entity... for me it seems weird (the difference in label vs name properties)

+++ b/core/modules/language/language.routing.yml
@@ -19,6 +19,20 @@ language_negotiation_selected:
+    _entity_form: 'language_entity.add'
...
+    _entity_create_access: 'language_entity'
...
+  pattern: '/admin/config/regional/language/edit/{language_entity}'
...
+    _entity_form: 'language_entity.edit'
...
+    _entity_access: 'language_entity.update'

+++ b/core/modules/language/lib/Drupal/language/Entity/Language.php
@@ -24,7 +24,11 @@
+ *       "add" = "Drupal\language\Form\LanguageAddForm",
+ *       "edit" = "Drupal\language\Form\LanguageEditForm"

Language entity faked with 'name' form element so language_save() could continue to work and form structure is not changed

Gábor Hojtsy’s picture

Yeah the add form and edit form edit/add entities, they should be entity form controllers as for any other config entity.

andypost’s picture

Status: Needs work » Needs review
FileSize
15.09 KB
9.09 KB

Re-roll for $this->t() and #title

Related #1946426: Convert all of confirm_form() in language.admin.inc to the new form interface uses entity too

andypost’s picture

FileSize
23.28 KB
8.19 KB

Missed the removal of old code

xjm’s picture

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

disasm’s picture

Status: Needs review » Reviewed & tested by the community

This one looks great and passes tests! Marking RTBC!

Gábor Hojtsy’s picture

Issue tags: +sprint

Agreed, let's get this in!

andypost’s picture

FileSize
23.28 KB

re-roll

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

Gábor Hojtsy’s picture

Thanks all!

Gábor Hojtsy’s picture

Yay, thanks all!

Status: Fixed » Closed (fixed)
Issue tags: +sprint

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

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks all!

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.