Project description

The Localized Configuration module is a custom layer on top of Drupal's configuration management system, allowing to build a centralized configuration interface for your website with reduced effort.

Localized Configuration does a lot of the footwork in regards to storing configuration and, most importantly, handles differences between different locales and languages in the Drupal environment, making it especially valuable if you use languages in a multisite context.

This module is intended as a framework for developers and does not provide meaningful functionality on its own for site-builders.

Application details

We believe the module to be a solid framework for developers who wish for an accessible, dynamic configuration system which can be made accessible to customers, and which centralizes the configuration for many custom features in a single place, as well as allowing customization per-language.

In our particular in-company use case, the Drupal concept of "language" ended up being used as more of a multisite context, with different domains and wholly different pieces of content. We have had multiple customers which desired for per-language customization of various settings, and we decided to come up with an extensible plugin system for that.

Project page

https://www.drupal.org/project/localized_config

Project documentation

https://www.drupal.org/docs/8/modules/localized-configuration

Git instructions

git clone --branch 8.x-1.x https://git.drupalcode.org/project/localized_config.git

PAReview.sh

https://pareview.sh/pareview/https-git.drupal.org-project-localized_conf...

The only message is that the project is missing unit tests, which the tool states to be "optional". Other code review topics have been handled in release 8.x-1.0-alpha2 in this tracker issue.

CommentFileSizeAuthor
#3 Screenshot 2019-12-19 at 11.00.00.png413.33 KBvuil

Comments

yoruvo created an issue. See original summary.

vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
commented
Issue summary: View changes

I updated summary.

vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
commented
Status: Needs review » Needs work
StatusFileSize
new Screenshot 2019-12-19 at 11.00.00.png413.33 KB

Thank you for the contribution!

1. Please correct in composer.json:
https.//www.drupal.org/u/yoruvo
to become
https://www.drupal.org/u/yoruvo

2. Please add the dependency to your project .info.yml file:

dependencies:
  - drupal:language
  - language_access:language_access

You have an undefined class \Drupal\language_access\LanguageAccessHelper in Drupal\localized_config\LocalizedConfigHelper?

3. Also add drupal/console as dependency in .info.yml file:

diff --git a/localized_config.info.yml b/localized_config.info.yml
index 63d1a9b..eeb0ee3 100644
--- a/localized_config.info.yml
+++ b/localized_config.info.yml
@@ -8,6 +8,8 @@ core_version_requirement: ^8 || ^9
 
 dependencies:
   - drupal:language
+  - drupal:console
+  - language_access:language_access
 
 configure: localized_config.settings

4. Please replace the deprecated function system_get_info(). Use \Drupal::service('extension.list.$type')->getExtensionInfo() or \Drupal::service('extension.list.$type')->getAllInstalledInfo() instead.
See more https://www.drupal.org/node/2709919

5. It is better (as a recommendation) to use if ($module !== null) {..} instead of if (!is_null($module)) {..} everywhere.

6. Please see the attachment about the issue into Drupal\localized_config\Form\LocalizedConfigForm create() method.

Not all of the issues above are blockers but it is good practice to know them.
Thank you.

yoruvo’s picture

yoruvo commented

Hi! Thank you very much for the review.

#1, #4 and #5 have been fixed.

I've addressed #2 and #6 by removing that dependency/class altogether; it was meant to be long gone.

As for #3, I'm not sure if I should have Drupal Console be a dependency, because the command is in no way required for the module to work. It is simply an optional aid for developers.

The new version of the module is up as 8.x-1.0-alpha6.

yoruvo’s picture

yoruvo commented
Status: Needs work » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Since Drupal Console is not necessary for the module to work, it should not be declared as required dependency. The Drupal Console command will be available to Drupal Console, if it's installed; otherwise, without Drupal Console, the command will not be visible.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community

Thanks for your contribution!

  1. pluginPermissions(): shouldn't each permission have a title and description property? The 'access localized config ...' don't seem to.
  2. config schema for the settings is missing, see https://www.drupal.org/docs/8/api/configuration-api/configuration-schema...
  3. "Class LocalizedConfigForm." is not a good class comment. Please describe what the class does or is used for instead. Please check all your class comments.
  4. "// Don't cache the form.": Why? A code comment should always describe the Why in addition to the What.
  5. drupal:localized_config is not a valid dependency because localized_config is not a Drupal core module. Should be localized_config:localized_config.

Otherwise looks good to me, did not see any obvious security issues.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno
Status: Reviewed & tested by the community » Fixed

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

Status: Fixed » Closed (fixed)

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