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
#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:

<?php
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
StatusFileSize
new6.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:

<?php
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
StatusFileSize
new4.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 ]
new7.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
StatusFileSize
new4.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.