OPENEUROPA-480: Provide a language selection page #5

richardcanoe · 2018-05-23T15:25:55Z

OPENEUROPA-480

Description

Add sub module to implement and configure language selection page module.

…isiting the frontpage.

pfrenssen · 2018-05-24T07:35:01Z

composer.json

-        "php": "^7.1"
+        "drupal/devel": "^1.2",
+        "drupal/language_selection_page": "2.2",
+        "drupal/pathauto": "1.2"


There are no good reasons to lock the versions of the Language Selection Page and Pathauto modules, unless there is a known problem with a newer version. In that case this should be documented here (e.g. by using a "_readme" entry), and followup issues should be made to fix the bug and unlock the version again.

Drupal modules are not yet following semantic versioning (but this is in the works, ref. https://www.drupal.org/project/drupal/issues/1612910), but they do follow the core principles of semantic versioning: new minor versions of modules should only contain bug fixes and new features which do not break backwards compatibility. This means that it can be considered safe to unlock the versions, and it is recommended because it allows us to benefit from bugfixes and security fixes without making any changes to the file.

Do we have a policy or rule on when to use ^2.2 and when to use ~2.2 ? The current file has a mixture of both.

@pfrenssen agreed, this will also allow site developers to fix the version they want to use in the site's composer.json, in case they need it, while if we lock them on the components' composer.json that would not be possible.

Regarding the ~2.2 and ^2.2 both are valid so they are both OK to be used. I don't think there is a need to define a policy because there is no "better" way to do this. Both cases are really easy to understand: the tilde updates only the most minor version specified, while the caret updates all versions except the most major one.

I personally prefer the tilde for dependencies that use a <major version>.<minor version> naming scheme such as Drupal modules. The caret is definitely the one to use for packages that adhere to semantic versioning <major version>.<minor version>.<patch>.

Some people like to use the caret for everything because it is "less confusing for newbies", but I think it is condescending to insinuate that young developers are not able to understand the difference between the two.

This has been addressed, thanks 👍

I see that the drupal/administration_language_negotiation dependency is also locked but this is out of scope for this ticket, I created a followup to do this: OPENEUROPA-601.

pfrenssen · 2018-05-24T07:42:44Z

composer.json

        "drupal/drupal-extension": "^4.0.0@alpha",
        "openeuropa/code-review": "^0.2",
        "openeuropa/task-runner": "^0.5",
-        "openeuropa/oe_theme": "dev-master",
+        "openeuropa/oe_theme": "dev-OPENEUROPA-480",


Before this PR is merged we should get this sister branch in OE Theme merged so this can be reverted back to the master branch.

I would also propose to not rely on the master branch but instead making regular releases for the OE Theme. It is clear here that we are relying on a new feature that is being developed in OE Theme: the template for the language switcher. Since we depend on that feature this should be clearly indicated in our dependency list. This information is missing if we say that we depend on dev-master. Composer could very well decide to check out a fork or a cached version of the repository that doesn't include the template. The only reliable way to do this is:

Accept and merge the sister PR in OE Theme.

Tag a new release for the OE Theme: eg. 0.1.1 or 0.2.0.

Indicate this version as the minimum version to use in our dependency graph: "openeuropa/oe_theme": "^0.1.1"

This way we can ensure that the actual template we depend on will be available, and Composer will refuse to build otherwise.

We would always commit the theme and remove the branch as part of the final commit process. I agree that now the platform is evolving and growing we should start using minimum version increases for components.

Yep, agreed on not using dev-master anymore and tagging theme releases more often, especially after openeuropa/composer-artifacts#1 will be merged.

This should be taken care of before this is merged into the master branch, or this WILL break the master when the sister PR in OE Theme is merged and the corresponding branch deleted. If we don't address this then the dev-OPENEUROPA-480 branch will not be resolved by Composer, and it will not be able to build the dependencies.

The best way to handle tickets that require dependent changes in multiple components is to make subtasks for each component, and handle them separately. This allows to push the dependent changes through first, then tag a new release and use this in the depending component.

pfrenssen · 2018-05-24T07:44:43Z

modules/oe_multilingual_selection_page/oe_multilingual_selection_page.info.yml

@@ -0,0 +1,19 @@
+name: 'OpenEuropa Multilingual Selection Page'
+type: module
+description: 'Module for OpenEuropa Multilingual Language Selection Page.'


It is better to provide a short description of what the module does, rather than parroting the name of the module.

This still needs to be addressed. Here's a suggestion for the description:

Provides a splash page that allows visitors to choose a language when they start browsing the website.

Actually this has been addressed in the latest version of the PR. Approved! 👍

pfrenssen · 2018-05-24T07:47:54Z

modules/oe_multilingual_selection_page/oe_multilingual_selection_page.info.yml

+    - field.field.node.oe_demo_translatable_page.field_oe_demo_translatable_body
+    - language.content_settings.node.oe_demo_translatable_page
+    - node.type.oe_demo_translatable_page
+    - pathauto.pattern.oe_demo_translatable_page


This defines 7 config files but they are not present in the PR? Were these forgotten?

I think this is a copy/paste gone wrong since they are exported in the demo module and should be kept there indeed.

Oh that explains it, thanks!

This has been fixed, thanks 👍

pfrenssen · 2018-05-24T07:49:37Z

modules/oe_multilingual_selection_page/oe_multilingual_selection_page.install

+      '/admin*',
+      '/node/add/*',
+      '/node/*/edit',
+    ])->save();


Instead of setting configuration in an install hook, the recommended way to provide default configuration is by exporting it into a YAML file and placing it in the config/install subfolder.

This configuration is not provided by the oe_multilingual_selection_page so it cannot be stored in its config/install module, doing that will result in an error at installation time.

config/optional then?

It would have the same effect, the configuration is in the config/install of the module this module depends on, so that will be installed first, causing the same error.

OK let's go with the install hook, but there is a risk involved with this. If the site builder enables this module, then decides not to use it and uninstalls it again, the change to the language negotiation will not be undone. I would not suggest to revert to the previous language negotiation strategy either, since there might be a very long time between the enabling and uninstallation of the module, and the site may have become dependent on the language negotiation provided by this module.

What I think we need is a disclaimer / opt-in checkbox when enabling the module, and also documentation in the README that explains to site builders that enabling this module will set the administrative language to English.

Is there anything we can revert safely on hook_uninstall()? Maybe we can remove the language_selection_page.negotiation part from config and the LanguageNegotiationLanguageSelectionPage::METHOD_ID from the language negotiation?

I don't think we can touch the other things on uninstall though :-/

Providing a disclaimer and opt-in checkbox on installation is out of scope for this ticket, let's do this in a followup after discussion. We can put a @todo here though linking to the followup, and make a note in the README.

pfrenssen · 2018-05-24T08:01:25Z

src/LanguageNegotiationSetter.php

+/**
+ * Helper service for language negotiation method configuration.
+ */
+class LanguageNegotiationSetter {


Let's provide an interface for this service so that it can be overridden by custom implementations while still ensuring that the API stays the same.

Great, there is an interface now, thanks a lot! 👍

pfrenssen · 2018-05-24T08:01:52Z

src/LanguageNegotiationSetter.php

@@ -0,0 +1,73 @@
+<?php
+
+namespace Drupal\oe_multilingual;


Lets declare strict typing.

This has been addressed, thanks! 👍

pfrenssen · 2018-05-24T08:02:03Z

src/LanguageNegotiationSetter.php

+   * @param array $methods
+   *   Array of language negotiation method names.
+   */
+  public function enableNegotiationMethods(array $methods) {


Return type declaration is missing.

This has been addressed, thanks! 👍

pfrenssen · 2018-05-24T08:02:55Z

src/LanguageNegotiationSetter.php

+   * Enable given language negotiation methods.
+   *
+   * @param array $methods
+   *   Array of language negotiation method names.


Maybe provide an example here of how the passed data should look? By reading this documentation I have no idea of the kind of method names I should be passing in.

OK there is ample documentation now, thanks! 👍

pfrenssen · 2018-05-24T08:03:25Z

tests/Behat/DrupalContext.php

+   * @param \Behat\Behat\Hook\Scope\BeforeScenarioScope $scope
+   *   The Hook scope.
+   *
+   * @BeforeScenario @selection-page


Very nice idea!

netlooker

I reviewed the PR and it looks good. Recent improvements are addressing most of the comments.

…d to current methods.

…l into OPENEUROPA-480

pfrenssen

Changes look good but we still need to fix the dependency on a PR branch in the OE Theme module, and mitigate the "dangerous" changes made to the language negotiation configuration when enabling the language selection page.

pfrenssen · 2018-05-29T10:14:24Z

composer.json

-        "php": "^7.1"
+        "drupal/devel": "^1.2",
+        "drupal/language_selection_page": "2.2",
+        "drupal/pathauto": "1.2"


This has been addressed, thanks 👍

I see that the drupal/administration_language_negotiation dependency is also locked but this is out of scope for this ticket, I created a followup to do this: OPENEUROPA-601.

pfrenssen · 2018-05-29T10:20:23Z

composer.json

        "drupal/drupal-extension": "^4.0.0@alpha",
        "openeuropa/code-review": "^0.2",
        "openeuropa/task-runner": "^0.5",
-        "openeuropa/oe_theme": "dev-master",
+        "openeuropa/oe_theme": "dev-OPENEUROPA-480",


This should be taken care of before this is merged into the master branch, or this WILL break the master when the sister PR in OE Theme is merged and the corresponding branch deleted. If we don't address this then the dev-OPENEUROPA-480 branch will not be resolved by Composer, and it will not be able to build the dependencies.

The best way to handle tickets that require dependent changes in multiple components is to make subtasks for each component, and handle them separately. This allows to push the dependent changes through first, then tag a new release and use this in the depending component.

pfrenssen · 2018-05-29T10:21:15Z

modules/oe_multilingual_selection_page/oe_multilingual_selection_page.info.yml

@@ -0,0 +1,19 @@
+name: 'OpenEuropa Multilingual Selection Page'
+type: module
+description: 'Module for OpenEuropa Multilingual Language Selection Page.'


Actually this has been addressed in the latest version of the PR. Approved! 👍

pfrenssen · 2018-05-29T10:21:55Z

modules/oe_multilingual_selection_page/oe_multilingual_selection_page.info.yml

+    - field.field.node.oe_demo_translatable_page.field_oe_demo_translatable_body
+    - language.content_settings.node.oe_demo_translatable_page
+    - node.type.oe_demo_translatable_page
+    - pathauto.pattern.oe_demo_translatable_page


This has been fixed, thanks 👍

pfrenssen · 2018-05-29T10:22:58Z

modules/oe_multilingual_selection_page/oe_multilingual_selection_page.install

+/**
+ * Implements hook_install().
+ */
+function oe_multilingual_selection_page_install() {


This has not yet been addressed, but it is such a minor issue that I won't block acceptance on this.

pfrenssen · 2018-05-29T10:26:45Z

modules/oe_multilingual_selection_page/oe_multilingual_selection_page.module

+    ->getCurrentLanguage()
+    ->getId();
+
+  foreach ($variables['language_links']['#items'] as $key => $value) {


This has been addressed, thanks! 👍

pfrenssen · 2018-05-29T10:27:45Z

src/LanguageNegotiationSetter.php

@@ -0,0 +1,73 @@
+<?php
+
+namespace Drupal\oe_multilingual;


This has been addressed, thanks! 👍

pfrenssen · 2018-05-29T10:28:06Z

src/LanguageNegotiationSetter.php

+/**
+ * Helper service for language negotiation method configuration.
+ */
+class LanguageNegotiationSetter {


Great, there is an interface now, thanks a lot! 👍

pfrenssen · 2018-05-29T10:29:08Z

src/LanguageNegotiationSetter.php

+   * Enable given language negotiation methods.
+   *
+   * @param array $methods
+   *   Array of language negotiation method names.


OK there is ample documentation now, thanks! 👍

pfrenssen · 2018-05-29T10:29:28Z

src/LanguageNegotiationSetter.php

+   * @param array $methods
+   *   Array of language negotiation method names.
+   */
+  public function enableNegotiationMethods(array $methods) {


This has been addressed, thanks! 👍

pfrenssen · 2018-05-29T10:33:00Z

OK so Github loses the context of my review comments because I have marked this as a "review" instead of normal comments. Very nice.

pfrenssen · 2018-05-29T10:38:33Z

I have done review +1 on the OE Theme change, this can now be tagged with a new release, so this part can be unblocked.

pfrenssen · 2018-05-29T10:39:01Z

There is a test failure due to insecure NPM dependencies, we need to address this too.

pfrenssen · 2018-05-29T10:43:21Z

Github is doing the worst possible job of displaying my review remarks. Here are links to the comments that require work:

…ge module.

pfrenssen · 2018-05-29T14:37:10Z

OK the disclaimer is good enough for me. Changes look good, will approve after test results are in.

brummbar · 2018-05-29T14:46:44Z

modules/oe_multilingual_selection_page/oe_multilingual_selection_page.module

+    $url = $value['#url']->toString();
+
+    $variables['languages'][] = [
+      "href" => $url,


We should use single quotes here.

brummbar · 2018-05-29T14:49:02Z

modules/oe_multilingual_selection_page/oe_multilingual_selection_page.module

+      "hreflang" => $language_code,
+      "label" => $value["#title"],
+      "lang" => $language_code,
+      "isActive" => $language_code === $current_language,


With ECL 1.0.0 this should be is_active.

richardcanoe and others added 6 commits May 22, 2018 15:05

OPENEUROPA-480: WIP.

ed46723

OPENEUROPA-480: Fix configuration import.

26d9978

OPENEUROPA-480: Refactor hook_install().

a440d50

OPENEUROPA-480: Add behat test.

7b2efeb

OPENEUROPA-480: Add theme branch to composer.

73eb7a8

OPENEUROPA-480: Make sure language selection page is available when v…

1148e4f

…isiting the frontpage.

pfrenssen suggested changes May 24, 2018

View reviewed changes

richardcanoe and others added 3 commits May 25, 2018 10:38

OPENEUROPA-480: Pull request fixes.

2522db4

OPENEUROPA-480: Removed dependency module install.

723dac9

Merge remote-tracking branch 'origin/master' into OPENEUROPA-480

bcf857b

netlooker previously approved these changes May 25, 2018

View reviewed changes

OPENEUROPA-480: Fixing typo to move tests on Drone further.

483e82a

netlooker dismissed their stale review via 483e82a May 28, 2018 13:32

ademarco added 2 commits May 28, 2018 16:14

OPENEUROPA-480: Make sure language selection page negotiation is adde…

8dd5578

…d to current methods.

Merge branch 'OPENEUROPA-480' of github.com:openeuropa/oe_multilingua…

f079c89

…l into OPENEUROPA-480

ademarco changed the title ~~Openeuropa 480~~ OPENEUROPA-480: Provide a language selection page May 28, 2018

OPENEUROPA-480: Add comment about optional module's configuration.

cd7662e

pfrenssen suggested changes May 29, 2018

View reviewed changes

ademarco added 4 commits May 29, 2018 15:52

Merge branch 'master' into OPENEUROPA-480

cb5f3df

OPENEUROPA-480: Set oe_theme version to ^0.2.

52c18fb

OPENEUROPA-480: Remove devel from composer requires.

46bb0fe

OPENEUROPA-480: Add README.md to OpenEuropa Multilingual Selection Pa…

da72ab3

…ge module.

brummbar suggested changes May 29, 2018

View reviewed changes

pfrenssen previously approved these changes May 29, 2018

View reviewed changes

OPENEUROPA-480: Add strict_types and remove obsolete isActive flag.

4465652

ademarco dismissed pfrenssen’s stale review via 4465652 May 29, 2018 14:56

brummbar approved these changes May 29, 2018

View reviewed changes

pfrenssen approved these changes May 29, 2018

View reviewed changes

ademarco merged commit 4465652 into master May 29, 2018

ademarco deleted the OPENEUROPA-480 branch May 29, 2018 15:39

OPENEUROPA-480: Provide a language selection page #5

OPENEUROPA-480: Provide a language selection page #5

Conversation

richardcanoe commented May 23, 2018

OPENEUROPA-480

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfrenssen May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlooker left a comment

Choose a reason for hiding this comment

pfrenssen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfrenssen commented May 29, 2018

pfrenssen commented May 29, 2018

pfrenssen commented May 29, 2018

pfrenssen commented May 29, 2018

pfrenssen commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfrenssen May 29, 2018 •

edited

Loading