Remove TranslationManager dependency from LanguageManager

Oh wow... I like this.

"Oh wow... I like this." (cit)

This is great, it removes the recursion that was bothering us in the var_export() and equality checks :)

yay for less circular dependencies circular less for yay

This would be good to test.

Working on tests.

Let's see if this fails.

So this is a bit more complex - I think we need to decouple the LanguageManager and TranslationManager completely. Discussed with @mr.baileys - I'm going to give this a shot.

The patch attached completely decouples TranslationManager from LanguageManager as well. This makes comparing TranslationWrappers much lighter. As the only injected objects it has are the Translators.

In order to do this we have to move TranslationManager::getNumberOfPlurals() to a service provided by locale - which makes perfect sense since it has little meaning of locale is not installed.

No interdiff provided because this is a major rework of the existing patch in #8 and it includes the tests (though converted to a KernelTestBase) from #15.

+++ b/core/includes/install.core.inc
@@ -346,11 +346,8 @@ function install_begin_request($class_loader, &$install_state) {
   $container
-    ->register('language_manager', 'Drupal\Core\Language\LanguageManager')
-    ->addArgument(new Reference('language.default'));
-  $container
     ->register('string_translation', 'Drupal\Core\StringTranslation\TranslationManager')
-    ->addArgument(new Reference('language_manager'));
+    ->addArgument(new Reference('language.default'));

This makes me happy. Less services required during very very early installer.

I discussed this new approach with @alexpott, and it makes total sense architecturally AFAIS.

Rerolled patch included an incorrect change in AccountForm that has since been changed in HEAD.

Updated to use TranslatableString and fixed up installer tests.

Fixing more tests

Fixing tests.

I really like the idea of decoupling!

1. +++ b/core/modules/locale/locale.services.yml
   @@ -14,6 +14,9 @@ services:
   +  locale.plural.forumla:
   +    class: Drupal\locale\PluralFormula
   

   I'm pretty convinced that forumla is the much nicer name. Did you solved the forumla? ... What are the forumlas needed to calculate the field of some juice

2. +++ b/core/tests/Drupal/KernelTests/Core/StringTranslation/TranslationStringTest.php
   @@ -0,0 +1,74 @@
   + */
   +class TranslationStringTest extends KernelTestBase {
   +
   

   I hand out beers for everyone doing that!

1. Fixed
2. I have my beer!

Edit: nevermind.

$success = $this->value === $other; from core/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php

Thank you!

+    // We also 2 plurals if there is no explicit information yet

Also? There's nothing before. And there's no verb.

+   * Loads the formulae and stores them on the object if not set.

+   * @return []

Stores on what object? And this doesn't return anything. "@return void" seems to be the way.

+   *   An array for formulas.

An array of formulae? Most of the patch uses the latin plural of formula (which I do not like but if you do I won't object too much) and let's be consistent. The "for" is almost surely wrong unless my English grammar is badly off. When I (emphasis on I) see an "array for formulae" I expect a by reference array which gets populated.

 // @todo

Sure, we can do a doxygen todo, I have no problems except one: catch said "naked" todos are useless and all of them should be accompanied by an issue. Please file one.

$this->formulae = $this->state->get('locale.translation.plurals', []);

locale.translation.formulae perhaps?

 difference services

different services.

Addressed #38

Formulas might be better, but not too bothered as this is only used on protected variables/methods and not part of the API.

By the way drush HEAD currently fails on drush si - I believe this patch would fix that?

Yes, drush + HEAD fails https://github.com/drush-ops/drush/pull/1619 issue + crude fix but this is better. Apparently if (isset($input[$element['#name']]) && $input[$element['#name']] == $element['#value']) { and $input[$element['#name']] becomes t('Save and continue') and then it crashes. So this is a better fix.

YAY! This fixed the fatal error I was getting when running drush si:

Fatal error: Nesting level too deep - recursive dependency? in /Users/phen/Sites/drupal8/core/lib/Drupal/Core/Form/FormBuilder.php on line 1310

Applying this patch corrects the issue. Looking at the code which triggered the fatal, the problem (and solution) make perfect sense. +1 RTBC.

I also get the same fatal from Drush as #43/#45/#50, and also fixed with this patch, so yay!

I want to review this a bit more thoroughly before committing myself, but no objection to another committer beating me to it.

This looks great, and because it fixes a fatal error when installing Drupal via Drush, I pushed it to 8.0.x without a change record. But I think we do need a change record for this, both explaining the general decoupling and specifically mentioning the methods removed from interfaces:

+++ b/core/lib/Drupal/Core/Language/LanguageManagerInterface.php
@@ -16,14 +16,6 @@
-  public function setTranslation(TranslationInterface $translation);
+++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
@@ -102,16 +102,4 @@ public function translateString(TranslatableString $translated_string);
-   public function getNumberOfPlurals($langcode = NULL);

Since this was commited I have some Exceptions in some modules like monitoring and simplenews, the exception says that the service added in this patch "locale.plural.formula" doesn't exist.
This is a problem in my modules or in the core due to this commit?

This is the complete Exception:

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException thrown while calling __toString on a Drupal\Core\StringTranslation\PluralTranslatableString object in /home/arla/public_html/d8/core/lib/Drupal/Component/DependencyInjection/Container.php on line 161: You have requested a non-existent service "locale.plural.formula".
trigger_error('Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException thrown while calling __toString on a Drupal\Core\StringTranslation\PluralTranslatableString object in /home/arla/public_html/d8/core/lib/Drupal/Component/DependencyInjection/Container.php on line 161: You have requested a non-existent service "locale.plural.formula".', 256)
Drupal\Core\StringTranslation\TranslatableString->__toString()
strtr('@method @url returned @status (@length).', Array)
Drupal\Component\Utility\SafeMarkup::placeholderFormat('@method @url returned @status (@length).', Array, 1)
Drupal\Component\Utility\SafeMarkup::format('@method @url returned @status (@length).', Array)
Drupal\simpletest\WebTestBase->curlExec(Array)
Drupal\simpletest\WebTestBase->drupalGet('cron/5YfS85PzSKeeqZLAULUzcmoG3iuU3heUpOKVQgRJNOf9ORTiHsObzDUEjbkrKFuuBP5ZOKILiw')
Drupal\simpletest\WebTestBase->cronRun()
Drupal\simplenews\Tests\SimplenewsAdministrationTest->testNewsletterIssuesOverview()
Drupal\simpletest\TestBase->run()
_simpletest_batch_operation(Array, '2', Array)
call_user_func_array('_simpletest_batch_operation', Array)
_batch_process()
_batch_do()
_batch_page(Object)
Drupal\system\Controller\BatchController->batchPage(Object)
call_user_func_array(Array, Array)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

This just hapens in local not in the testbot (I'm running it in PHP 5.6).

I think the problem is in PluralTranslatableString::getPluralIndex(). That checks the existence of the function, and that function then calls out to that service. But if the parent site has locale enabled then that module is loaded and there is nothing you can do about that.

Sounds like we need to change that to a module exists?

Opened #2573975: function_exists check in PluralTranslatableString is wrong

That needs follow-up

1. +++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
   @@ -70,12 +70,17 @@ protected function formatPluralTranslated($count, $translated, array $args = arr
   -   * See the
   -   * \Drupal\Core\StringTranslation\TranslationInterface::getNumberOfPlurals()
   +   * See the \Drupal\locale\PluralFormulaInterface::getNumberOfPlurals()
       * documentation for details.
   +   *
   +   * @see \Drupal\locale\PluralFormulaInterface::getNumberOfPlurals()
   

   duplicates each other

2. +++ b/core/modules/locale/src/PluralFormula.php
   @@ -0,0 +1,116 @@
   +  protected $formulae;
   ...
   +  protected function loadFormulae() {
   +    if (!isset($this->formulae)) {
   

   formula(e)?
   suppose 'formulas'

Filled follow-up #2613976: Remove redundant "See..." that duplicates "@see..." in protected method documentation

change records can and should still be able to be updated, and since that is why this issue is still open, adding a tag so we can find this.

adding link to instructions on how to draft a change record to the issue summary remaining tasks section
https://drupal.org/contributor-tasks/draft-change-record

Please, can we get a CR for this?

Drafted a CR at https://www.drupal.org/node/2801819

@vijaycs85: this happened before 8.0.0, so the branch for 8.1.x was not correct. Also fixed a header problem: https://www.drupal.org/node/2801819/revisions/view/10067211/10070045

As @effulgentsia said though

I think we do need a change record for this, both explaining the general decoupling and specifically mentioning the methods removed from interfaces:

that is not covered in the change entry yet.

Thanks @Gábor Hojtsy for the review. I have updated the CR to reflect @effulgentsia comment.

Reviewed, updated a bit. Looks all done now :) Moving back to the right version.

Also finally removing from the sprint.