Files: 
CommentFileSizeAuthor
#20 core-language-elements-test-routing-2086499-20.patch5.87 KBphiit
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-language-elements-test-routing-2086499-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 interdiff-2086499-16-20.txt983 bytesphiit
#16 core-language-elements-test-routing-2086499-16.patch5.88 KBphiit
FAILED: [[SimpleTest]]: [MySQL] 58,468 pass(es), 17 fail(s), and 6 exception(s).
[ View ]
#16 interdiff-2086499-14-16.txt1.69 KBphiit
#14 core-language-elements-test-routing-2086499-14.patch5.67 KBphiit
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#14 interdiff-2086499-6-14.txt1.82 KBphiit
#6 core-language-elements-test-routing-2086499-6.patch6.09 KBphiit
PASSED: [[SimpleTest]]: [MySQL] 59,184 pass(es).
[ View ]
#6 interdiff-2086499.txt3.22 KBphiit
#2 core-language-elements-test-routing-2086499-2.patch7.44 KBphiit
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Comments

Assigned:Unassigned» phiit

Working on this

Assigned:phiit» Unassigned
Status:Active» Needs review
StatusFileSize
new7.44 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

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.

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

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.

Assigned:phiit» Unassigned
Status:Needs work» Needs review
StatusFileSize
new3.22 KB
new6.09 KB
PASSED: [[SimpleTest]]: [MySQL] 59,184 pass(es).
[ View ]

Fixed class names on routing. Interdiff attached for comparison.

Status:Needs review» Reviewed & tested by the community

@phiit, this looks good now.

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?

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?

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

Assigned:Unassigned» phiit

Continued working on this issue.

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.

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

StatusFileSize
new1.82 KB
new5.67 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Empty methods removed.

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!

StatusFileSize
new1.69 KB
new5.88 KB
FAILED: [[SimpleTest]]: [MySQL] 58,468 pass(es), 17 fail(s), and 6 exception(s).
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new983 bytes
new5.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-language-elements-test-routing-2086499-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ahh alright, changes made.

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

Status:Needs review» Reviewed & tested by the community

Cool, let's do this.

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.

Status:Needs work» Closed (duplicate)