function_exists check in PluralTranslatableString is wrong

We have an existing issue to write a test to run tests with language/locale modules are enabled.

Added related issues.

1. +++ b/core/lib/Drupal/Core/StringTranslation/PluralTranslatableString.php
   @@ -146,7 +146,8 @@ public function render() {
    
   -    $index = $this->getPluralIndex();
   +    $langcode = isset($this->options['langcode']) ? $this->options['langcode'] : NULL;
   +    $index = $this->getStringTranslation()->getPluralIndex($this->count, $langcode);
        if ($index == 0) {
   

   Can we not keep using $this->getOption('langcode') for this, like the removed code?

2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
   @@ -102,4 +102,19 @@ public function translateString(TranslatableString $translated_string);
   +   *
   +   * @param $count
   +   *   Number to return plural for.
   +   * @param $langcode
   +   *   Optional language code to translate to a language other than
   +   *   what is used to display the page.
   +   *
   +   * @return
   +   *   The default translation manager returns -1 because Locale is not
   +   *   installed.
   

   Nitpicks:

   * missing types
   * the return description is a bit strange as it just documents the special case now.
   * (optional)

3. +++ b/core/modules/locale/locale.services.yml
   @@ -37,3 +37,9 @@ services:
   +  locale.string_translation:
   +    decorates: string_translation
   +    class: Drupal\locale\TranslationManager
   +    decoration_inner_name: string_translation.inner
   +    arguments: ['@string_translation.inner']
   +    public: false
   

   Interesting. Wondering what happens if someone tries to replace that from a site specific services.yml. AFAIK, later yml files just autoamtically replace earlier definitions (core is always the first I think), not sure if it's a good idea to rely on that or not.

4. +++ b/core/modules/locale/src/TranslationManager.php
   @@ -0,0 +1,119 @@
   +/**
   + *
   + */
   +class TranslationManager implements TranslationInterface, TranslatorInterface {
   +  use DependencySerializationTrait;
   +
   +  public function __construct(TranslationInterface $string_translation) {
   +    $this->stringTranslation = $string_translation;
   +  }
   

   .

5. +++ b/core/modules/locale/src/TranslationManager.php
   @@ -0,0 +1,119 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function translate($string, array $args = array(), array $options = array()) {
   +    $safe = TRUE;
   

   Did you just copy the full class or is the plan to simplify the core implementation do not actually do any translation?

Looking good to me.

I think we're still missing explicit test coverage for this, e.g. what #2097185: Add tests to test SimpleTest with language module enabled on the parent site would provide and possibly merge that patch in here.

We already decided once before to postpone test coverage for that when that issue was opened, that it never happened shows that we should probably not get this in without a regression test.

Needs rebase.
Actually this test #2097185: Add tests to test SimpleTest with language module enabled on the parent site shows that this issue change the errors that gets, I think this fixes part of the errors that we get in the test.

I'll do the rebase and merge with the tests.

Rebased. There is still some failing tests that I don't know how to fix.

I don't know why, but if I change the test to one that doesn't use WebTestBase like LanguageFallbackTest, then the test doesn't fail, but the problem is without the patch it also don't fail, so then it doesn't really help us.

I think to trigger this scenario, we need a test that does *not* enable locale in a parent site that does. So we need two tests, with different modules.

We also can't just run any test because a child site test apparently needs that special .htkey file to be able to do requests against the child-child site. So added a new test that does just that. Still doesn't fail as expected, though :(

Uploading the current progress and including test-only patch that doesn't seem to fail for me.

I have this error today and for some reason I thought Berdir could help me ;).
After patching I had a fatal but a readable one.

GET /admin/structure/types/manage/article/display?destination=/node/1 returned 0 (0 bytes).	Browser	ManageDisplayTabTest.php	28	Drupal\ds\Tests\ManageDisplayTabTest->testFieldPlugin()

The actual test line was the following:

$this->drupalGet('node/' . $node->id() . '/display');

That route does a redirect to the URL above.

I know this is probably not related but maybe it gives an idea of the cause of the issue.

Updating issue summary because it wasn't clear it was talking about running tests.

Tempted to bump to critical as suggested in #27. You basically can't run simpletests from multilingual sites and expect them to work.

By the way, that latest patch seems way way over complicated.

See #2583697-3: Fatal error running some tests in UI with locale.module enabled for a suggestion of a simpler way to fix this -- just check that the service exists before calling into it, rather than assuming it does exist. This seems way simpler than making a whole new class etc.

As far as making a test for this... could we not take a different tactic and make a new testing *environment* for it? I think we are already testing vs. PostgreSQL and various versions of PHP for Drupal 8. Could we also make a different test script that would enable all Drupal modules in the outer environment, and run all the tests from that environment? That might catch some other similar problems. I don't know enough about the testing infrastructure I guess, but an approach like that might be easier than trying to coerce simulation of running a test within another test.

OK, that makes sense.

I still don't know if we can make an automated test for this though, since it's a case of "If you run this test from a site that has module A installed, the way the test runner works makes the test break", not "If you do this on a site, it is broken and the test fails to illustrate the bug" like most tests. The attempt at making a test so far has failed.

Which is why I was wondering if we could make a test environment that would run tests from a site that has more modules enabled (like maybe all of them), to see if any other problems come up. It is possible that locale having this particular problem is not the only one in Core like this, where the assumption is that if a function exists, a certain module is enabled (and thus its services exist.

Also regarding the latest patch... I thought that if Core provides a service, and a module wants to override the service (when it is enabled), then the right way was ... see
https://api.drupal.org/api/drupal/core!core.api.php/group/container/8#se...
(the section on overriding services). That doesn't seem to be what the locale module is doing in this patch?

Hm. We don't seem to have any documentation on service "decorating". What is that and where is it documented? Guess that is a separate issue...

Re-rolling.

I can reproduce this issue when running more than one test of GitHub's port of Pathauto. Without this patch, the following error happens when running more than one test (and Locale module not being installed):

[Thu Dec 24 19:52:23.556232 2015] [:error] [pid 2673] [client
127.0.0.1:58628] User error:
Symfony\\Component\\DependencyInjection\\Exception\\ServiceNotFoundException
thrown while calling __toString on a
Drupal\\Core\\StringTranslation\\PluralTranslatableMarkup object in
/var/www/drupal8/core/lib/Drupal/Component/DependencyInjection/Container.php
on line 161: You have requested a non-existent service
"locale.plural.formula". in
/var/www/drupal8/core/lib/Drupal/Component/Utility/ToStringTrait.php on
line 25, referer: http://d8.local/batch?id=41&op=start

After applying the patch, I don't see this error when I run Pathauto tests.

Hi, I'm getting this on a completely clean install with the minimal profile --

Message	User error: Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException thrown 
while calling __toString on a Drupal\Core\StringTranslation\PluralTranslatableMarkup object in 
/Users/jgrubb/drupal/path-dups/core/lib/Drupal/Component/DependencyInjection/Container.php on line 161: 
You have requested a non-existent service "locale.plural.formula". in Drupal\Core\StringTranslation\TranslatableMarkup->__toString() 
(line 25 of /Users/jgrubb/drupal/path-dups/core/lib/Drupal/Component/Utility/ToStringTrait.php).

I do not have any multi-lingual modules enabled and am trying to run everything in the "system" group from the UI.

Needs issue summary update with proposed resolution, API changes, etc. Also given the API changes (new methods required on an interface), I don't think its allowed in 8.0.x, but it would need to be 8.1.x, no?

Yep, the current patch on this issue is 8.1.x material. There might be a way that we could make an 8.0.x-safe patch, but moving to 8.1.x for now.

As I understand semver, we'll have to make an 8.0.x safe patch any way we cut this. No?

Eclipse

Oops, I forgot to add a file into my re-roll patch at #42 (TranslationManager.php). Here is the full patch. There is still work to do to meet @Gábor Hojtsy's and @xjm's feedback.

Discussed this further with @xjm. The matter of fact is Drupal 8.1.x does not allow backwards incompatible changes to interfaces either, such as requiring new methods to be implemented, which would mean a WSOD with existing class implementations. See https://www.drupal.org/core/d8-allowed-changes#minor My understanding then is the proposed change as-is is only possible in Drupal 9 and a new interface needs to be added extending the existing one to fix this issue.

Removed a debug statement and fixed a t() statement in the tests.

I am working now on a patch that complies with semantic versioning for 8.0.x.

Okay I misunderstood @Gábor Hojtsy's question on the issue. Looking at the current patch, it cannot go into 8.1.x either because there are changes to an interface. We need to find a way to fix this bug that does not break BC, including not changing existing interfaces. See https://www.drupal.org/core/d8-allowed-changes#minor and https://www.drupal.org/core/d8-bc-policy for details on what can go into a minor release.

@EclipseGc and @juampynr, not all fixes go into 8.0.x at all. I'd suggest only focusing on an 8.1.x-compatible fix for now.

I must be missing something, but this alternative patch works for me locally. I could run Pathauto tests without experiencing the reported error. Let's see what core tests say.

Look simple and even 8.0.x compatible to me :) Let's add a test in core then that this fixes the problem.

The static doesn't make sense to me. We can't statically cache a non-static state that might change during the request. If this works, then only because nothing calls it before calling it within the test and nothing calls it after that we'd expect to work again.

But yes, something like that is the quickfix that we have to apply here then. But I'd actually do the hasService() within locale_get_plural() then, because then it's statically cached with drupal_static() which is invalidated before/after running tests.

Note that we have tried to write tests for this before and failed to produce a test that fails/passes as expectedly.

This is a pretty serious issue, that everyone with a non-english site faces when running tests and it's very hard to understand. Not sure if we should push a fix in without test coverage. We did that before (#2097185: Add tests to test SimpleTest with language module enabled on the parent site) and it is now broken again, but fixing it might be more important here than preventing regressions.

Here is another patch where I have removed the static variable static::$localeEnabled (as far as I understand, it is not needed) and I have moved the hasService() check intolocale_get_plural().

I don't know how to replicate this issue with tests because part of the required context resides in the environment that triggers the tests (having locale module installed).

Here is the simplest way to replicate it:

1. Install locale module.
2. Run Drupal\aggregator\Tests\AggregatorAdminTest

Expected: tests pass.
Actual: The following errors are triggered because the test calls \Drupal::translation()->formatPlural:

Here is the textual version of the error:

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException thrown while calling __toString on a Drupal\Core\StringTranslation\PluralTranslatableMarkup object in /var/www/drupal8/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\PluralTranslatableMarkup object in /var/www/drupal8/core/lib/Drupal/Component/DependencyInjection/Container.php on line 161: You have requested a non-existent service "locale.plural.formula".', 256)
Drupal\Core\StringTranslation\TranslatableMarkup->__toString()
Drupal\Core\Datetime\DateFormatter->formatInterval(900)
array_map(Array, Array)
Drupal\aggregator\Entity\Feed::baseFieldDefinitions(Object)
Drupal\Core\Entity\EntityFieldManager->buildBaseFieldDefinitions('aggregator_feed')
Drupal\Core\Entity\EntityFieldManager->getBaseFieldDefinitions('aggregator_feed')
Drupal\Core\Entity\EntityFieldManager->getFieldStorageDefinitions('aggregator_feed')
Drupal\Core\Entity\EntityManager->getFieldStorageDefinitions('aggregator_feed')
Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->__construct(Object, Object, Object, Object)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->getStorageSchema()
Drupal\Core\Entity\Sql\SqlContentEntityStorage->Drupal\Core\Entity\Sql\{closure}()
Drupal\Core\Entity\Sql\SqlContentEntityStorage->wrapSchemaException(Object)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->onEntityTypeCreate(Object)
Drupal\Core\Entity\EntityTypeListener->onEntityTypeCreate(Object)
Drupal\Core\Entity\EntityManager->onEntityTypeCreate(Object)
Drupal\Core\Entity\EntityDefinitionUpdateManager->installEntityType(Object)
Drupal\Core\Extension\ModuleInstaller->install(Array, 1)
Drupal\Core\ProxyClass\Extension\ModuleInstaller->install(Array, 1)
Drupal\simpletest\WebTestBase->installModulesFromClassProperty(Object)
Drupal\simpletest\WebTestBase->setUp()
Drupal\aggregator\Tests\AggregatorTestBase->setUp()
Drupal\simpletest\TestBase->run()
_simpletest_batch_operation(Array, '30', 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)

YES, #57 spotlights the problem exactly. I think that checking function_exists() could also cause weirdness where a function could get called (in the parent site PHP) that shouldn't exist in the test environment because the module isn't enabled. If we're using function_exists() as a proxy for "is this module installed that defines the function", we should instead check that the module is installed.

However, the problem is probably not that bad. For instance, we do function_exists() to invoke hooks, but we already only check for invoking them on installed modules, or in other cases like while we're installing a module. We also do function_exists() to check for PHP extensions, and those don't differ between the test running environment and the in-test site.

Anyway... maybe we should do a survey of all the function_exists() calls in Core and see if they're OK [wrapped inside some module exists check or checking PHP extension type things] or need attention.... I'll take a look.

OK... So I grepped through core for function_exists() calls, and looked at all of them. I decided it was probably OK if a test was looking for function_exists() on a testing module, because no one would have that enabled anyway. And it was probably OK if a call was made to test for a PHP library like Curl or GD or multibyte or stuff like that. And I think the calls in ModuleHandler and ThemeHandler* classes are OK because they're wrapped inside checks for which modules/themes are enabled.

So. Here's the remaining list of possibly questionable function_exists() calls, for your perusal. Some of them may not be problematic... some probably are, for cases of "Running test A from an environment where test B is enabled".

I am not clear on how should I adjust the patch. @alexpott's feedback at #56 says:

The problem with #55 is that locale_get_plural() will then statically cache results when the service is not available.

I think that I don't understand the above statement. By debugging I could see that locale_get_language() caches the plural formula for all languages. Should I change anything there?

On an additional note, this was my initial statement for debugging the workflow until locale_get_language() is called: echo \Drupal::translation()->formatPlural(2, '1 comment', '@count comments');

Right, and there's one in there that would do the same for the History module:

modules/comment/tests/src/Unit/CommentLinkBuilderTest.php:  if (!function_exists('history_read')) {

And several that I think are calling into File module functions, or they could be the base file.inc functions...

Hi, just wanted to say that the patches from #52 and #55 both fix the problem that I was having - not being able to run the "system" test group from the UI or the "path" test group from the UI. It was bombing out with the same error that @juampy posted at the top of #55 - the supposed ajax error that was actually caused by the subject of this thread.

Thanks all.

Thanks @jhodgdon, I inspected the following for each of your findings a) what does the code that has the function_exists() statement do with it; and b) what does the function being checked by function_exists() do:

Here are my findings:

includes/common.inc: if (function_exists('_drupal_maintenance_theme')) {

Seems safe to me. The theme layer is always present.

lib/Drupal/Core/StringTranslation/PluralTranslatableMarkup.php: static::$localeEnabled = function_exists('locale_get_plural');

This is the one that got us into this issue. It is problematic because locale_get_plural() calls \Drupal::service('locale.plural.formula')->getFormula($langcode);, which may not be available in a test.

lib/Drupal/Core/StringTranslation/PluralTranslatableMarkup.php: if (function_exists('locale_get_plural')) {

Same as the above one.

lib/Drupal/Core/Controller/ControllerResolver.php: if (function_exists($controller)) {

Safe. It just returns $controller.

modules/comment/tests/src/Unit/CommentLinkBuilderTest.php: if (!function_exists('history_read')) {

Safe. It is mocking the function.

modules/language/tests/src/Unit/LanguageNegotiationUrlTest.php: if (!function_exists('base_path')) {

Safe. It is mocking the function.

modules/node/src/Plugin/views/row/Rss.php: elseif (function_exists('rdf_get_namespaces')) {

Safe. It does not load any service that may not be available in a test.

modules/file/file.module: if (function_exists($function)) {

Safe, it's just calling the function.

modules/views/tests/src/Unit/EntityViewsDataTest.php: if (!function_exists('t')) {

Safe. It is mocking the function.

modules/views/tests/src/Unit/Routing/ViewPageControllerTest.php: if (!function_exists('views_add_contextual_links')) {

Safe. It is mocking the function.

modules/views/tests/src/Unit/Plugin/Block/ViewsBlockTest.php: if (!function_exists('views_add_contextual_links')) {

Safe. It is mocking the function.

modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php: if (!function_exists('base_path')) {

Safe. It is mocking the function.

modules/views/src/Plugin/views/row/RssFields.php: if (function_exists('rdf_get_namespaces')) {

Safe. It does not load any service that may not be available in a test.

tests/Drupal/KernelTests/KernelTestBase.php: if (function_exists('drupal_static_reset')) {

Safe. It does not load any service that may not be available in a test.

tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php:if (!function_exists('file_create_url')) {

Safe. It is mocking the function.

tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php:if (!function_exists('file_uri_scheme')) {

Safe. It is mocking the function.

tests/Drupal/Tests/Core/Asset/CssCollectionRendererUnitTest.php: if (!function_exists('file_create_url')) {

Safe. It is mocking the function.

tests/Drupal/Tests/Core/Session/PermissionsHashGeneratorTest.php: if (!function_exists('user_role_permissions')) {

Safe. It is mocking the function.

tests/Drupal/Tests/Core/Render/Element/MachineNameTest.php: if (!function_exists('t')) {

Safe. It is mocking the function.

As @alexpott said, once #2312191: Change Simpletest UI from a test runner to a CLI snippet generator gets fixed, this issue won't be a problem again as there will be full separation between the environment running tests and the test environment.

Should I work on anything else on the latest patch?

Thanks for the analysis in #66, but... The problem is not just that a service wouldn't exist. The problem is also this...

For example, let's think about function_exists('rdf_get_namespaces').

Let's say you are running tests from a site that has the RDF module enabled, and running a test that doesn't include the RDF module. This function exists check, when running inside a test, will return TRUE. However, inside the test site, the RDF module is not enabled, so the TRUE return value is wrong. The test can call the function, but it will probably not function properly (no database for RDF in the test site, etc.).

OK, so. We still have this very commonly encountered problem that tests have trouble running from the Simpletest UI if either the locale/language modules are being used in the test and not in the site the tests are running from, or they are being used on the site the tests are running from, but not in the test (I have seen problems both ways).

The above patch fixes this problem, apparently (see #65), but it may need some adjustment (see #56) and it also doesn't have a test because it's really hard to make a test that catches this problem in the simpletest UI.

Also, there are probably other problems in core (see #60).

So. Questions:

a) Does this issue have to fix all possible problems like this in Core? Or can we fix the problem many people are currently having, and maybe do all the rest in a follow-up issue?

b) Can we commit this patch that would fix the immediate problem, without a test and even though it may have other problems?

This is really getting in the way of development, at least for me.

I don't think all the function_exists should be fixed in this issue. Also given that we don't see a way to test this, I think this should get in and let people move forward. However the analysis done on the other cases of function_exists is good and issues should be opened to fix those too. The solution may be entirely different for those.

Patch at #74 works for me.

Looks good to me too

+++ b/core/lib/Drupal/Core/StringTranslation/PluralTranslatableMarkup.php
@@ -157,10 +150,13 @@ public function render() {
+    // We have to test both if the function exists and the service since in certain

nitpick - over 80 chars

Patch at #75 still applies.

+++ b/core/lib/Drupal/Core/StringTranslation/PluralTranslatableMarkup.php
@@ -150,7 +150,13 @@ public function render() {
-    if (function_exists('locale_get_plural')) {
+    // We have to test both if the function exists and the service since in certain
+    // certain situations it is possible that locale code might be loaded but
+    // the service does not exist. For example, where the parent test site has

More than a nitpick. certain is doubled.

@lokapujya: can you help with the nitpick and more than nitpick so we can get this in? :) that would be amazing :) Thanks!

Nitpicked.

Yay, thanks!

Committed/pushed to all three 8.x branches, thanks!

Yay, thanks all!