Language domain may not be left blank for default language

I am not sure where the domain is taken when on a translated domain. I mean how would Drupal tell which domain to use? Whether it is en.x.y.z or just x.y.z? Just specify the value for default language and let's see if that works. If so, we should fix the docs.

Right I believe the docs are not correct. Drupal has no way to tell the default domain unless provided. Can you help with a patch? Should just remove that sentence AFAIS.

We changed the form alter, to check for a domain for each language. In addition the description on the form is altered and the error message, on the form validate is altered aswell. see the attached patch.

I'm looking at why this patch failed testing.

@alimac said is going to try and run the tests locally to get some more info.

-

1.

+++ b/core/modules/language/src/Form/NegotiationUrlForm.php
@@ -130,10 +130,10 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
           // Throw a form error if the domain is blank for a non-default language,
           // although it is required for selected negotiation type.

this comment needs to change also.

2.
there are no changes to tests. Maybe we did not have test about this? Or maybe we did and now they are failing cause we did not update them...

Maybe those tests were not setting the default domain. and so now there is an exception thrown during the tests.

3.
It might be interesting to do a git blame on those lines which say it may be blank to try and find out why we thought it would be a good idea to allow no value.

I ran tests locally and a lot more of them failed. Running tests again to see if the results will be consistent.

This patch will most likely fail, but it should fail a smaller number of tests than the previous patch.

We should add a test for when the exception is thrown (that is, when the domain for default language is not specified). Also, noticed some typos in the doc blocks.

Test failed earlier because we supplied an explicit default hostname (en.example.com). Instead, since we don't know the hostname, let's try using gethostname() to determine the current hostname.

+++ b/core/modules/language/src/Form/NegotiationUrlForm.php
@@ -59,7 +59,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      '#description' => $this->t('The domain names to use for these languages. The domain may not be left blank. Use with caution in a production environment.<strong>Modifying this value may break existing URLs. Use with caution in a production environment.</strong> Example: Specifying "de.example.com" as language domain for German will result in an URL like "http://de.example.com/contact".'),

"Use with caution.." is repeated twice.
Previously, "Leave blank" used active voice, so we should also use active voice: https://www.drupal.org/style-guide/content#voice
Maybe: Don't leave the domain blank.

This patch has the fix for the remaining test (LanguageUrlRewritingTest) and also updated text of the description.

Still need to see about writing a test for leaving the domain field blank.

Here's a test to make sure that if you leaven the domain field blank, it will throw an error.

Fixed a whitespace issue and added an assertion for the test in which configuration is saved successfully.

Adding a test-only patch that should fail.

Good. The patch passed and the test-only patch failed with the expected message.

I'm working on this issue

@xadag Please assign the issue to yourself while working on an issue. It will prevent others to start woking on the issue.

@Sutharsan new contributors often work together on issues, so we don't assign issues. Instead we post comments to communicate what we are going to do ("I am going to manually test this patch at the DrupalCon Amsterdam mentored sprint" for example.)

Ok, thanks.

Hello,

I've reviewed the latest two patches (full one and only testing one).
Everything is fine for me, process and code, when now i'm viewing a translated node in english and switched to spanish with the language selector, i've the correct version and domain displayed.

When i'm trying to submit the configuration of detection domain with blank domain for default language, i've got an error so everything it's fine.

I tried the patch.
Switch on the language module.
Add a Hungarian language
Go to Configure URL language negotiation page.
Change "Part of the URL that determines language" to "Domain"
Save empty fields.

Without patch, I can save emty English domain. With patch I got a following error:

No, I will check the code, and if I do not find any issue, I put it to rtbc

The line:

'domain[en]' => gethostname(),

missing in test only patch.

I reroll that patch and waiting for testbot.
Anything else looking good for me.

If it will be FAILED, I put it to RTBC

oooh... I must to goo. Anybody else must put it to RTBC, sorry

@pp

The test-only patch is supposed to only add the assertion, and not the 'domain[en]' => gethostname() part, because it's testing what happens when the domain for the default language (in this case 'en') is left blank.

We wanted to see to see what changed from the test only patch in the #24 to the the test only patch in #37.
I made the interdiff by applying the patch no #24 the 8.0.x on branch 24 then switching back to 8.0.x and applying patch no #37 and running a diff between the two branches.

No the test only patch needs to be equal to the test changes of the complete fix. Otherwise how do you know that it properly fails? The test looks good, it has coverage for both empty and filled in domains, it should properly fail without the fix. It *should not* be a different test.

Already done in #37 so no need to set to needs work.

So I used the test module to run the 2 tests changed by this patch and also did a manual test. And the output by the test module was ok for the empty domain issue.
However another testline returned a failure. I guess it can be caused by my local environment not allowing it.example.com. I don't know if this is relevant.
If not. I think it is RTBC.

P.S. I've added a screenshot of the test moduel output.

Please update the tags and the issue summary, specifically the remaining tasks.

+++ b/core/modules/language/src/Form/NegotiationUrlForm.php
@@ -59,7 +59,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      '#description' => $this->t('The domain names to use for these languages. Leave blank for the default language. Use with caution in a production environment.<strong>Modifying this value may break existing URLs. Use with caution in a production environment.</strong> Example: Specifying "de.example.com" as language domain for German will result in an URL like "http://de.example.com/contact".'),
...
       '#states' => array(

I do not like the double negative in: "Do not leave the domain blank." Either make it a positive sentence about required values or remove this part completely as its added value is limited.

removed 'Do not leave the domain blank.' per #43

A am manualy testing this right now.

Here is the screenshot of manual testing before applying patch #44.

+++ b/core/modules/language/src/Form/NegotiationUrlForm.php
@@ -59,7 +59,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-      '#description' => $this->t('The domain names to use for these languages. Leave blank for the default language. Use with caution in a production environment.<strong>Modifying this value may break existing URLs. Use with caution in a production environment.</strong> Example: Specifying "de.example.com" as language domain for German will result in an URL like "http://de.example.com/contact".'),
+      '#description' => $this->t('The domain names to use for these languages. <strong>Modifying this value may break existing URLs. Use with caution in a production environment.</strong> Example: Specifying "de.example.com" as language domain for German will result in an URL like "http://de.example.com/contact".'),

If the value field is no longer optional, shouldn't we add '#required' => TRUE, here?

screen shot after:

@Xano The field cannot be required at this point because the domain fields are hidden and only appear when the user selects Domain radio button. (This form could be refactored but that's outside of the scope of this issue.)

Tested the before and after situation manually. The test went succesfully.
Before applying the patch leaving the english domain empty was allowed.
After applying the patch #44 it wasn't allowed and the correct error message was displayed.

@Xano The field cannot be required at this point because the domain fields are hidden and only appear when the user selects Domain radio button. (This form could be refactored but that's outside of the scope of this issue.)

Thanks! It makes total sense not to mark this field as required.

Manual testing was done in #52. Multiple people have confirmed the patch works, that this approach is the right one, and that the test coverage is sufficient. The code looks good. This is good to go!

I' tested this on:
http://testbot.drupal-pt.org/node/404

You have the results here with 1 fail:
http://testbot.drupal-pt.org/sites/default/files/build_D80x_mysql55_PHP5...

Also you can have the results suite from:
http://testbot.drupal-pt.org/sites/default/files/build_D80x_mysql55_PHP5...

Tested:
DCI_TESTGROUPS="language,locale,system"

Committed and pushed to 8.0.x. LIVE FROM DRUPALCON! :D

Follow up for the broken testbot: https://www.drupal.org/node/2350543