-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OPENEUROPA-480: Provide a language selection page #5
Changes from 9 commits
ed46723
26d9978
a440d50
7b2efeb
73eb7a8
1148e4f
2522db4
723dac9
bcf857b
483e82a
8dd5578
f079c89
cd7662e
cb5f3df
52c18fb
46bb0fe
da72ab3
4465652
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
name: 'OpenEuropa Multilingual Selection Page' | ||
type: module | ||
description: 'Generates a Language Selection Page used on first access to a multilingual site.' | ||
core: 8.x | ||
package: 'OpenEuropa' | ||
|
||
dependencies: | ||
- drupal:language_selection_page | ||
- oe_multilingual:oe_multilingual |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
<?php | ||
|
||
/** | ||
* @file | ||
* Install, update and uninstall functions for the module. | ||
*/ | ||
|
||
declare(strict_types = 1); | ||
|
||
use Drupal\administration_language_negotiation\Plugin\LanguageNegotiation\LanguageNegotiationAdministrationLanguage; | ||
use Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUrl; | ||
use Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationSelected; | ||
use Drupal\language_selection_page\Plugin\LanguageNegotiation\LanguageNegotiationLanguageSelectionPage; | ||
|
||
/** | ||
* Implements hook_install(). | ||
*/ | ||
function oe_multilingual_selection_page_install() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing return type declaration ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has not yet been addressed, but it is such a minor issue that I won't block acceptance on this. |
||
// Configure selection page language negotiation method. | ||
\Drupal::configFactory() | ||
->getEditable('language_selection_page.negotiation') | ||
->set('path', '/select-language') | ||
->set('type', 'standalone') | ||
->set('ignore_neutral', FALSE) | ||
->set('blacklisted_paths', [ | ||
'/admin', | ||
'/user', | ||
'/admin/*', | ||
'/admin*', | ||
'/node/add/*', | ||
'/node/*/edit', | ||
])->save(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This configuration is not provided by the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 |
||
|
||
/** @var \Drupal\oe_multilingual\LanguageNegotiationSetterInterface $setter */ | ||
$setter = \Drupal::service('oe_multilingual.language_negotiation_setter'); | ||
|
||
// For interface negotiation make sure administrative pages are in English. | ||
$setter->setInterfaceSettings([ | ||
LanguageNegotiationAdministrationLanguage::METHOD_ID => -20, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be removed. We should not mess with the site's language negotiation settings simply because the site builder is enabling this small submodule. This could unexpectedly break the language negotiation strategy which is usually very carefully defined by the stakeholders, without a clear indication of what caused this sudden change, and without a way to revert the damage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As explained in the previous comment, let's keep it for now but try to handle whatever we can revert safely in a |
||
LanguageNegotiationUrl::METHOD_ID => -19, | ||
LanguageNegotiationLanguageSelectionPage::METHOD_ID => -18, | ||
LanguageNegotiationSelected::METHOD_ID => 20, | ||
]); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
<?php | ||
|
||
/** | ||
* @file | ||
* Contains hooks to manipulate language links for selection page. | ||
*/ | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's declare strict typing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not yet done. |
||
/** | ||
* Implements hook_page_preprocess(). | ||
*/ | ||
function oe_multilingual_selection_page_preprocess_language_selection_page_content(array &$variables) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Return type declaration is missing ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not yet done but is a minor issue which doesn't block acceptance. |
||
|
||
$current_language = \Drupal::languageManager() | ||
->getCurrentLanguage() | ||
->getId(); | ||
|
||
// Generate an array suitable for use in the ecl-language-list component. | ||
foreach ($variables['language_links']['#items'] as $key => $value) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this code doing? There is no documentation here to indicate why this is needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has been addressed, thanks! 👍 |
||
$language_code = $key; | ||
$url = $value['#url']->toString(); | ||
|
||
$variables['languages'][] = [ | ||
"href" => $url, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use single quotes here. |
||
"hreflang" => $language_code, | ||
"label" => $value["#title"], | ||
"lang" => $language_code, | ||
"isActive" => $language_code === $current_language, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With ECL 1.0.0 this should be |
||
]; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,29 +36,31 @@ function oe_multilingual_install() { | |
'/node/*/translations', | ||
])->save(); | ||
|
||
// Make sure that English language prefix is set to "en". | ||
\Drupal::configFactory() | ||
->getEditable('language.negotiation') | ||
->set('url.prefixes.en', 'en') | ||
->save(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems strange, we set the language negotiation for English, but leave the rest untouched? It seems clear to me that the goal of OE Multilingual is to provide the full language negotiation tactic. Just overriding English won't be enough since there are no guarantees that the other settings are matching the defaults from Drupal core. In other words, this will work fine only if the site builder hasn't been messing with the language negotiation before enabling this module. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other language settings are not coming from core but from our module's About the site builder messing with configuration: we won't have this case since the OpenEuropa components are meant to be enabled at the beginning of the development process, the site configuration will be then exported in the site's I do agree, however, that this might generate confusion, we should either document this or have a talk with the team about this, there is also a ticket about configuration management, we could deal with this in that ticket's scope. |
||
|
||
/** @var \Drupal\oe_multilingual\LanguageNegotiationSetterInterface $setter */ | ||
$setter = \Drupal::service('oe_multilingual.language_negotiation_setter'); | ||
|
||
// Set default language negotiation methods. | ||
$setter->enableNegotiationMethods([ | ||
LanguageInterface::TYPE_INTERFACE, | ||
LanguageInterface::TYPE_CONTENT, | ||
]); | ||
|
||
// For interface negotiation make sure administrative pages are in English. | ||
$interface_settings = [ | ||
$setter->setInterfaceSettings([ | ||
LanguageNegotiationAdministrationLanguage::METHOD_ID => -20, | ||
LanguageNegotiationUrl::METHOD_ID => -19, | ||
LanguageNegotiationSelected::METHOD_ID => 20, | ||
]; | ||
]); | ||
|
||
// For content negotiation make sure that content respects URL language. | ||
$content_settings = [ | ||
$setter->setContentSettings([ | ||
LanguageNegotiationUrl::METHOD_ID => -19, | ||
LanguageNegotiationSelected::METHOD_ID => 20, | ||
]; | ||
|
||
// Set default language negotiation methods with related weights. | ||
\Drupal::configFactory() | ||
->getEditable('language.types') | ||
->set('configurable', [ | ||
LanguageInterface::TYPE_INTERFACE, | ||
LanguageInterface::TYPE_CONTENT, | ||
]) | ||
->set('negotiation.' . LanguageInterface::TYPE_INTERFACE . '.enabled', $interface_settings) | ||
->set('negotiation.' . LanguageInterface::TYPE_INTERFACE . '.method_weights', $interface_settings) | ||
->set('negotiation.' . LanguageInterface::TYPE_CONTENT . '.enabled', $content_settings) | ||
->set('negotiation.' . LanguageInterface::TYPE_CONTENT . '.method_weights', $content_settings) | ||
->save(); | ||
]); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
services: | ||
oe_multilingual.language_negotiation_setter: | ||
class: Drupal\oe_multilingual\LanguageNegotiationSetter | ||
arguments: ['@config.factory'] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
<?php | ||
|
||
declare(strict_types = 1); | ||
|
||
namespace Drupal\oe_multilingual; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets declare strict typing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has been addressed, thanks! 👍 |
||
|
||
use Drupal\Core\Config\ConfigFactoryInterface; | ||
use Drupal\Core\Language\LanguageInterface; | ||
|
||
/** | ||
* Helper service for language negotiation method configuration. | ||
*/ | ||
class LanguageNegotiationSetter implements LanguageNegotiationSetterInterface { | ||
|
||
/** | ||
* Configuration name. | ||
*/ | ||
const CONFIG_NAME = 'language.types'; | ||
|
||
/** | ||
* Drupal\Core\Config\ConfigFactoryInterface definition. | ||
* | ||
* @var \Drupal\Core\Config\ConfigFactoryInterface | ||
*/ | ||
protected $configFactory; | ||
|
||
/** | ||
* Constructs a new LanguageNegotiationSetter object. | ||
*/ | ||
public function __construct(ConfigFactoryInterface $config_factory) { | ||
$this->configFactory = $config_factory; | ||
} | ||
|
||
/** | ||
* Enable given language negotiation methods. | ||
* | ||
* @param array $methods | ||
* Array of language negotiation method names. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK there is ample documentation now, thanks! 👍 |
||
*/ | ||
public function enableNegotiationMethods(array $methods):void { | ||
$this->configFactory | ||
->getEditable(self::CONFIG_NAME) | ||
->set('configurable', $methods) | ||
->save(); | ||
} | ||
|
||
/** | ||
* Set interface language negotiation settings. | ||
* | ||
* @param array $settings | ||
* Array of language negotiation method names with their weights. | ||
*/ | ||
public function setInterfaceSettings(array $settings):void { | ||
$this->configFactory | ||
->getEditable(self::CONFIG_NAME) | ||
->set('negotiation.' . LanguageInterface::TYPE_INTERFACE . '.enabled', $settings) | ||
->set('negotiation.' . LanguageInterface::TYPE_INTERFACE . '.method_weights', $settings) | ||
->save(); | ||
} | ||
|
||
/** | ||
* Set content language negotiation settings. | ||
* | ||
* @param array $settings | ||
* Array of language negotiation method names with their weights. | ||
*/ | ||
public function setContentSettings(array $settings):void { | ||
$this->configFactory | ||
->getEditable(self::CONFIG_NAME) | ||
->set('negotiation.' . LanguageInterface::TYPE_CONTENT . '.enabled', $settings) | ||
->set('negotiation.' . LanguageInterface::TYPE_CONTENT . '.method_weights', $settings) | ||
->save(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
<?php | ||
|
||
declare(strict_types = 1); | ||
|
||
namespace Drupal\oe_multilingual; | ||
|
||
/** | ||
* Helper service for language negotiation method configuration. | ||
*/ | ||
interface LanguageNegotiationSetterInterface { | ||
|
||
/** | ||
* Enable given language negotiation methods. | ||
* | ||
* @param array $methods | ||
* Array of language negotiation method names. | ||
*/ | ||
public function enableNegotiationMethods(array $methods):void; | ||
|
||
/** | ||
* Set interface language negotiation settings. | ||
* | ||
* @param array $settings | ||
* Array of language negotiation method names with their weights. | ||
*/ | ||
public function setInterfaceSettings(array $settings):void; | ||
|
||
/** | ||
* Set content language negotiation settings. | ||
* | ||
* @param array $settings | ||
* Array of language negotiation method names with their weights. | ||
*/ | ||
public function setContentSettings(array $settings):void; | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ | |
|
||
namespace Drupal\Tests\oe_multilingual\Behat; | ||
|
||
use Behat\Behat\Hook\Scope\AfterScenarioScope; | ||
use Behat\Behat\Hook\Scope\BeforeScenarioScope; | ||
use Behat\Gherkin\Node\TableNode; | ||
use Drupal\Core\Entity\ContentEntityInterface; | ||
use Drupal\DrupalExtension\Context\RawDrupalContext; | ||
|
@@ -15,6 +17,33 @@ | |
*/ | ||
class DrupalContext extends RawDrupalContext { | ||
|
||
/** | ||
* Enable OpenEuropa Multilingual Selection Page module. | ||
* | ||
* @param \Behat\Behat\Hook\Scope\BeforeScenarioScope $scope | ||
* The Hook scope. | ||
* | ||
* @BeforeScenario @selection-page | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very nice idea! |
||
*/ | ||
public function setupSelectionPage(BeforeScenarioScope $scope): void { | ||
\Drupal::service('module_installer')->install(['oe_multilingual_selection_page']); | ||
} | ||
|
||
/** | ||
* Disable OpenEuropa Multilingual Selection Page module. | ||
* | ||
* @param \Behat\Behat\Hook\Scope\AfterScenarioScope $scope | ||
* The Hook scope. | ||
* | ||
* @AfterScenario @selection-page | ||
*/ | ||
public function revertSelectionPage(AfterScenarioScope $scope): void { | ||
\Drupal::service('module_installer')->uninstall([ | ||
'oe_multilingual_selection_page', | ||
'language_selection_page', | ||
]); | ||
} | ||
|
||
/** | ||
* Create content given its type and fields. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
<?php | ||
|
||
namespace Drupal\Tests\oe_multilingual\Behat; | ||
|
||
use Behat\MinkExtension\Context\RawMinkContext; | ||
|
||
/** | ||
* Class MinkContext. | ||
*/ | ||
class MinkContext extends RawMinkContext { | ||
|
||
/** | ||
* Assert that visitor is redirected to language selection page. | ||
* | ||
* @Then I should be redirected to the language selection page | ||
*/ | ||
public function assertLanguageSelectionPageRedirect() { | ||
$this->assertSession()->addressMatches("/.*\/select-language/"); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:"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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, agreed on not using
dev-master
anymore and tagging theme releases more often, especially after openeuropa/composer-artifacts#1 will be merged.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.