Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phiit’s picture

Assigned: Unassigned » phiit

Working on this

phiit’s picture

Assigned: phiit » Unassigned
Status: Active » Needs review
FileSize
7.44 KB

Here goes my first patch, be gentle!

Status: Needs review » Needs work

The last submitted patch, core-language-elements-test-routing-2086499-2.patch, failed testing.

heddn’s picture

phiit, good work on your first patch. Its close. I noticed a few whitespace issues. Always good to catch these as it makes the committer's life a lot easier. Plus there's a fatal error with the name of the form classes. These need to match what's in the routing.yml file.

+++ b/core/modules/language/tests/language_elements_test/lib/Drupal/language_elements_test/Form/LanguageConfigurationElement.php
@@ -0,0 +1,57 @@
+class LanguageConfigurationElement extends FormBase {
+
+  /**
+     * {@inheritdoc}
+     */
+  public function getFormID() {
+      return 'language_elements_configuration_element';
+  }

There are some whitespace issues with the @inheritdoc comment and the return clause, use only 2 spaces.

+++ b/core/modules/language/tests/language_elements_test/lib/Drupal/language_elements_test/Form/LanguageConfigurationElementTest.php
@@ -0,0 +1,46 @@
+ * @file
+ * Contains \Drupal\language_elements_test\Form\LanguageConfigurationElementTestForm.
+ */
+
+namespace Drupal\language_elements_test\Form;
+
+use Drupal\Core\Form\FormBase;
+
+/**
+ * A form containing a language select element.
+ */
+class LanguageConfigurationElementTest extends FormBase {

The class name needs to match whats in the routing.yml file. Otherwise you get nasty errors:

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "\drupal\language_elements_test\form\languageconfigurationelementform" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 875 of /home/lucas/websites/drupal/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

phiit’s picture

Assigned: Unassigned » phiit

Ahh, got it. Yeah I wasn't that sure about the class names, I'll give it another go. And somehow I missed a bunch of these whitespaces errors from c&p.

phiit’s picture

Assigned: phiit » Unassigned
Status: Needs work » Needs review
FileSize
3.22 KB
6.09 KB

Fixed class names on routing. Interdiff attached for comparison.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

@phiit, this looks good now.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/language/tests/language_elements_test/lib/Drupal/language_elements_test/Form/LanguageConfigurationElement.php
@@ -0,0 +1,57 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function validateForm(array &$form, array &$form_state) {
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function submitForm(array &$form, array &$form_state) {

+++ b/core/modules/language/tests/language_elements_test/lib/Drupal/language_elements_test/Form/LanguageConfigurationElementTest.php
@@ -0,0 +1,46 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function validateForm(array &$form, array &$form_state) {
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function submitForm(array &$form, array &$form_state) {

Hm. If we're not implementing any code in those methods, I don't think these should be here. Else you're going to end up canceling the upstream validate/submit code that's happening in FormBase, no?

phiit’s picture

I left them there as the class extending FormBase need to implement these methods or they need to be declared abstract. How should I proceed with the new patch?

Gábor Hojtsy’s picture

Right, the problem is formBase is an abstract class that does not even have a submitForm implementation although it says it implements FormInterface. Also validateForm is empty in the abstract class. I guess we can call parent implementations. Not sure if it would blow up if the parent lacks that implementation...

phiit’s picture

Assigned: Unassigned » phiit

Continued working on this issue.

phiit’s picture

Assigned: phiit » Unassigned

From the same meta issue:
Issue #1979034: Convert views_test_data_element_form() to a Controller has the same empty methods committed.
Issue #2035689: Convert user_form_test_current_password to a Controller doesn't have validateForm method and it's committed.

I've verified that the methods submitForm and validateForm are indeed abstract. Which way is preferred in this case? a) leave them out or b) use empty methods or c) do something else.

tstoeckler’s picture

@phiit: What I just told you in person was actually completely wrong as I completely misunderstood this issue.

+++ b/core/modules/language/tests/language_elements_test/lib/Drupal/language_elements_test/Form/LanguageConfigurationElement.php
@@ -0,0 +1,57 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function validateForm(array &$form, array &$form_state) {
+  }

+++ b/core/modules/language/tests/language_elements_test/lib/Drupal/language_elements_test/Form/LanguageConfigurationElementTest.php
@@ -0,0 +1,46 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function validateForm(array &$form, array &$form_state) {
+  }

This can be removed, it's already in FormBase

Re @webchick: FormBase does not actually do any validation and submitting. Also there's a test in core that checks that the language element submit actually is called. So the empty submit should be fine as is.

phiit’s picture

tstoeckler’s picture

Ahh, sorry for not being precise. The submitForm() actually needs to stay, because that's not in FormBase. Only validateForm() should go. Could you re-roll for that real quick? That would be awesome!

phiit’s picture

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community

Yes, great. Thanks a lot!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, core-language-elements-test-routing-2086499-16.patch, failed testing.

tstoeckler’s picture

So this has been sitting for a while and core changed in the meantime:

+++ b/core/modules/language/tests/language_elements_test/language_elements_test.routing.yml
@@ -0,0 +1,13 @@
+  pattern: '/language-tests/language_configuration_element'
...
+  pattern: '/language-tests/language_configuration_element_test'

This should be:

  path: '/language....'
phiit’s picture

Status: Needs work » Needs review
FileSize
983 bytes
5.87 KB

Ahh alright, changes made.

tstoeckler’s picture

Yay, thanks. Let's see if the bot likes this...

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Cool, let's do this.

YesCT’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +D8MI, +language-content, +FormInterface, +WSCCI-conversion

The last submitted patch, core-language-elements-test-routing-2086499-20.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Closed (duplicate)
tim.plunkett’s picture

Status: Needs review » Needs work

The last submitted patch, 27: language-2086499-27.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.35 KB

Needed PSR-4 switch.

tim.plunkett’s picture

FileSize
6.53 KB

And removed dead code.

The last submitted patch, 29: language-2086499-29.patch, failed testing.

tim.plunkett’s picture

FileSize
1.52 KB
6.5 KB

Removed the $pid per Jibran's review

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Simple enough.

Gábor Hojtsy’s picture

Issue tags: +sprint

Yay, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5106b55 and pushed to 8.x. Thanks!

  • alexpott committed 5106b55 on 8.x
    Issue #2086499 by phiit, tim.plunkett | Gábor Hojtsy: Convert two page...
Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks all!

Status: Fixed » Closed (fixed)

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