It should be good to have some clear standard for this question, raised first on this issue #1785086: Introduce a generic API for interface translation strings, but it would affect any other storage controller like the ones created in entity_get_controller() (entity.inc)

As a side issue, I'd like to clarify too what is the right way to define the class to be used for the storage controller when there is'nt hook_info() to declare it, like in that locale_storage() function in the linked issue.

Files: 
CommentFileSizeAuthor
#22 1806756-22.patch3.29 KBYogesh Pawar
#17 1806756-17.patch4.41 KBchishah92
#13 1806756-13.patch4.34 KBrpayanm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#11 interdiff.txt7.81 KBalansaviolobo
#11 how_to_register_storage-1806756-11.patch4.39 KBalansaviolobo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#3 locale_storage_dic-01.patch6.03 KBJose Reyero
FAILED: [[SimpleTest]]: [MySQL] 41,809 pass(es), 26 fail(s), and 13 exception(s). View

Comments

Jose Reyero’s picture

Title: Do storage controllers need a 'target' database parameter? (Looking for a common standard) » How to register storage controllers / 'target' database parameter (Looking for a common standard)
Category: feature » support

It seems the answer is in Drupal\Core\CoreBundle.php

/**
 * Bundle class for mandatory core services.
 *
 * This is where Drupal core registers all of its services to the Dependency
 * Injection Container. Modules wishing to register services to the container
 * should extend Symfony's Bundle class directly, not this class.
 */

Thus we'll need to create a LocaleBundle class, and register the storage like this:

class LocaleBundle extends Bundle
{
  public function build(ContainerBuilder $container) {
    $container->register('locale.storage', 'Drupal\locale\StringDatabaseStorage')
    ->addArgument(new Reference('database'))
    ->addArgument(array('target' => 'default'));
  }
}

This seems to work and be a clean usage of DIC. More feedback welcomed though.

Note: if you couldn't find any documentation either: Classes in mymodule/lib/Drupal/mymodule/MymoduleBundle are automatically loaded by Drupal core from DrupalKernel::registerBundles(). Note the capital 'M' in 'Mymodule'

Jose Reyero’s picture

That approach above seems to work, but not for install/update, for which the storage seems not to be created, though locale_storage() is invoked causing a DIC exception. This may be related to #1807272: Mixed locale issues with upgrade scripts, unit tests, 'Table simpletest..locales_source doesn't exist', etc...

Jose Reyero’s picture

Category: support » feature
Status: Active » Needs review
Issue tags: +D8MI
FileSize
6.03 KB
FAILED: [[SimpleTest]]: [MySQL] 41,809 pass(es), 26 fail(s), and 13 exception(s). View

This is the working patch atm for locale storage controller. This is quite some special case, as noted in the locale_storage() function's comments so we need to handle the case of no locale storage present in DIC too.

tstoeckler’s picture

Issue tags: -D8MI

I don't think the second argument is necessary. If I wanted locale.storage to use a non-default database, I would do the following:

class LocaleBundle extends Bundle {

  public function build(ContainerBuilder $container) {
    $container->register('database.some_target', 'Drupal\Core\Database\Connection')
      ->setFactoryClass('Drupal\Core\Database\Database')
      ->setFactoryMethod('getConnection')
      ->addArgument('some_target');
    $container->getDefinition('locale.storage')
      ->replaceArgument(0, new Reference('database.some_target'));
  }
}

That way local.storage doesn't need to know about database targets.

tstoeckler’s picture

Issue tags: +D8MI

Restoring tag. The previous was a x-post.

tstoeckler’s picture

Oh, I just saw, that DatabaseStringStorage already takes an $option parameter, so #4 is bogus. (Or if at all, a follow-up.)

Status: Needs review » Needs work

The last submitted patch, locale_storage_dic-01.patch, failed testing.

Jose Reyero’s picture

Well, the patch fails but it actually fixes some issues. We get now real exceptions instead of "another exception thrown while handling one exception", which was happending before (because of #1808864: Exception handling is not 'locale safe' (thus not database safe))

This error is triggered by javascript file translation from the installer and would be in principle easy to fix, though this may not be the best solution:

--- a/core/modules/locale/locale.module
+++ b/core/modules/locale/locale.module
@@ -414,6 +414,12 @@ function locale_system_update($components) {
  * file if necessary, and adds it to the page.
  */
 function locale_js_alter(&$javascript) {
+  // If during install or update, we don't do anything if we don't have locale
+  // storage availabe.
+  if (!locale_storage()) {
+    return FALSE;
+  }
+
   $language_interface = language(LANGUAGE_TYPE_INTERFACE);
 
   $dir = 'public://' . variable_get('locale_js_directory', 'languages');

I'll be exploring other options like initializing the storage on locale_language_init(). Though the indicated place to do it is LocaleBundle, this seems not to be installer/update safe, which is too bad :-(

Jose Reyero’s picture

After some more research, it seems the locale storage is not a good candidate for a clean initialization because of the reasons outlined here, https://drupal.org/node/1807272#comment-6595134

jair’s picture

Issue tags: +Needs reroll

Needs reroll

alansaviolobo’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
4.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
7.81 KB

Status: Needs review » Needs work

The last submitted patch, 11: how_to_register_storage-1806756-11.patch, failed testing.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
4.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

rerolling...

Status: Needs review » Needs work

The last submitted patch, 13: 1806756-13.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

chishah92’s picture

Assigned: Unassigned » chishah92
chishah92’s picture

Status: Needs work » Needs review
FileSize
4.41 KB

Rerolled the patch.

Status: Needs review » Needs work

The last submitted patch, 17: 1806756-17.patch, failed testing.

chishah92’s picture

Assigned: chishah92 » Unassigned

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hitesh-jain’s picture

Assigned: Unassigned » hitesh-jain
Yogesh Pawar’s picture

Status: Needs work » Needs review
FileSize
3.29 KB

I have rerolled the patch to work with 8.2.x branch.

Status: Needs review » Needs work

The last submitted patch, 22: 1806756-22.patch, failed testing.