Fixed: Undefined property originalLanguageDefault in simpletest.

[bradbulger]

In backdrop_web_test_case_cache.php, the tearDown() function of BackdropWebTestCaseCache refers to an originalLanguageDefault property. That property is not defined.

It's a little murky. It was originally added to BackdropWebTestCase by the fix to #1835. Then in issue #2776, a fix to the Drupal issue 1823504 was applied, and that ended up removing the property. I think maybe only because it had moved? Or something else inadvertent.

This matters because I wonder if the proper fix is to restore some of the changes from #1835.

You can see this by running SimpleTestBrokenSetUp ("Broken SimpleTest method") and displaying errors and/or checking the error log.

How to reproduce

1. Enable Simpletest module
2. Make sure, test caching is on (admin/config/development/testing/settings)
3. If you ran tests before, clean test environment (admin/config/development/testing)
4. Run a test via UI, for instance "Language list configuration"
5. Visit admin/reports/dblog

Actual result

There are three dblog entries with:
Notice: Undefined property: BackdropWebTestCaseCache::$originalLanguageDefault in BackdropWebTestCaseCache->tearDown() (line 160 of .../core/modules/simpletest/backdrop_web_test_case_cache.php).

[indigoxela]

Drupal 7 defines it as such: $this->originalLanguageDefault = variable_get('language_default');

In Backdrop this variable can get accessed with config_get('system.core', 'language_default'); or language_default().

[indigoxela]

Here's a PR which sets originalLanguageDefault in function prepareEnvironment().

[bradbulger]

Is the code that is using this doing something that makes sense now? Isn't $GLOBALS['conf'] a Drupal thing - old style variable_set()?

The original code was checking the config

$this->originalLanguageDefault = config_get('system.core', 'language_default');

and then restoring that value if there was one, but it wasn't restoring it to the config. I don't know if test code like this

foreach ($languages as $key => $value) {
  // Set the default language.
  // Get the system config.
      $core_config = config('system.core');
      $core_config->set('language_default', $key);
  $core_config->save();

is affecting the actual config files of the installation, and so needs a config_set() to put it back?

[indigoxela]

Is the code that is using this doing something that makes sense now?

Yes, I think so. The purpose of originalLanguageDefault is not to fiddle with the actual config files, but to restore the global in $conf back to the original in function tearDown, in case it has been changed. (Backdrop and Drupal 7 globals are mostly the same.)

language_default() does the same as config_get('system.core', 'language_default'), but with a static cache and a fallback.

[bradbulger]

That's what I'm saying, though. For one, there is no global $conf. That's Drupal variables. For another, the original code that created this line was only setting it if it had found a value in the system.core config. It returned it to the status quo.

Are the config changes made in tests affecting the actual config of the site? Or only the copies? If it's not changing the real system.core config, then isn't the simplest thing to remove this altogether?

[indigoxela]

there is no global $conf

There it is: https://api.backdropcms.org/api/backdrop/core%21authorize.php/global/conf/1

See also: https://github.com/backdrop/backdrop/blob/5aba056ecbcaa20db3292fc70c0556fa1a178044/core/includes/bootstrap.inc#L569

See also: https://github.com/backdrop/backdrop/blob/5aba056ecbcaa20db3292fc70c0556fa1a178044/core/includes/bootstrap.inc#L1106

See also: https://api.backdropcms.org/api/backdrop/core%21includes%21drupal.inc/group/drupal_compatibility/1

If it's not changing the real system.core config...

Simpletest does not change any real config files. The global $conf is available in all scopes throughout a script, which is probably the reason, why it's reset after running tests.

[bradbulger]

I think it ends up "restoring" values that weren't there to begin with, and it would be fine to remove these lines altogether.

Otherwise, I think it's simpler to just add
$this->originalLanguageDefault = config_get('system.core', 'language_default');
to setUp().

[klonos]

@indigoxela if the simple one-liner fixes this problem here, then I would like us to get it in, to get another thing out of the way. Can you please rebase your PR?

[klonos]

I have tried the STR from the issue summary:

• in a recent Backdrop demo sandbox -> reproduces the 3 notices in the log 👍🏼
• in the PR sandbox -> throws 2 same exceptions: EXCEPTION: copy(files/simpletest/579166/config_active/.htaccess) [function.copy]: failed to open stream: Permission denied (will test again, once the PR has been rebased) after the PR was rebased, the test run w/o any errors/exceptions, and no notices in the log 👍🏼
• on a recently-cloned 1.x + the PR changes merged in -> fixes the issue 👍🏼 (no exceptions - no notices in the log)

I'm tentatively marking this as WFM, pending rebase in order to mark it RTBC.

[indigoxela]

I'm tentatively marking this as WFM, pending rebase in order to mark it RTBC.

Rebased, and tests are ... "passing".

[herbdool]

I've read all the comments and I didn't feel there was consensus on the fix. I'm not clear on any implications. It looks like an easy fix but it's confusing how the different things were added and removed around this.

@bradbulger and @indigoxela do you think you'd come to agreement on the fix here?

[indigoxela]

@herbdool regarding the previous comments here - yes, we came to agreement to open a new issue for the wider change and discussions regarding how checkPath is supposed to work - to not further block this important bugfix. Sorry, unrelated.

[herbdool]

@indigoxela sorry I'm still a bit confused. This issue is about originalLanguageDefault but you linked to checkPath.

[indigoxela]

sorry I'm still a bit confused...

You are absolutely right, another example of "read before you post" - sorry about the confusion. (Same person, different issue.)

If you have concerns, you maybe better ask @bradbulger - I just provided the bugfix. It they think, this requires further discussion, then maybe I'll just close my PR to make space for alternate approaches.

[herbdool]

@indigoxela please keep your PR open. It might be the best one here but it wasn't clear to me based on your discussion above. I investigated a bit @bradbulger's concerns too but nothing conclusive.

[herbdool]

I've got a PR removing originalLanguageDefault. It sounds like we can safely remove file_public_path too so I added that to the PR.

[indigoxela]

It sounds like we can safely remove file_public_path

Strange, there's a test failure in CommentApprovalTest with php 7.4:

filemtime(): stat failed for modules/ddd_installer_test/ddd_installer_test.info

Could this be related to that change?

[indigoxela]

I did a quick local check, running only that test on php 7.4. It passes locally, so probably the failure is unrelated.

@herbdool can you please close and reopen the PR to verify (or, if you have permission, just re-run the workflow)?

That test used to be one of the random failures, but got fixed years ago.

[herbdool]

@indigoxela thanks, I didn't know I had permissions to rerun the tests. I've just done so.

[herbdool]

@indigoxela looks like it was just a random failure

[bugfolder]

Tested per original instructions: worked to suppress the dblog entries. WFM.

Code reviewed. Looks good.

RTBC.

[quicksketch]

Thanks! I merged backdrop/backdrop#3825 into 1.x and 1.20.x.

Props to @herbdool, @bradbulger, @indigoxela, @klonos, and @bugfolder for their work on this issue!