Actually test node_access_test_secret_catalan and clean up deprecated code in NodeAccessLanguageTest [#2175739]

NodeAccessLanguageTest has a call to variable_set for node_access_test_secret_catalan that is incorrect as the module that uses this has been converted to state. Also that is no test that actually explicitly tests the effect of node_access_test_secret_catalan. One issue with the tests is that we don't check for the opposite condition.

Making a major as I'm removing a variable set.

Comment	File	Size	Author
#5	2175739.5.patch	9.13 KB	alexpott
#5	1-5-interdiff.txt	1.2 KB	alexpott
#1	2175739.1.patch	8.88 KB	alexpott
#1	2175739.1.will-fail.patch	8.81 KB	alexpott
#1	interdiff.txt	800 bytes	alexpott

Comments

Comment #1

alexpott

he/they

English

🇪🇺🌍

commented 17 January 2014 at 13:56

Status:	Active	» Needs review
Issue tags:		+Configuration system

Status	File	Size
new	interdiff.txt	800 bytes
new	2175739.1.will-fail.patch	8.81 KB
new	2175739.1.patch	8.88 KB

This test is had to read because it uses bad variable names and creates things it never actually tests on. For example in testNodeAccessPrivate it creates $node that it never actually uses and it calls $node_public = $this->drupalCreateNode(array('body' => array(array()), 'langcode' => 'hu', 'private' => TRUE)); - the whole point is that this node is not actually public :)

The patch attached:

replaces the variable_set with the correct call to the state system
removes deprecated calls to entity_access_controller()
removes calls to drupal_static_reset('node_access'); this is just incorrect
and improves the tests to actually test both sides of changing node_access_test_secret_catalan

A "will fail" version is also attached where all the refactoring to the tests is still there but the old variable_set remains. The interdiff is between the two patches.

This patch is easier to review by applying and reading the test rather than looking at the patch.

Comment #2

17 January 2014 at 15:28

The last submitted patch, 1: 2175739.1.will-fail.patch, failed testing.

Comment #3

ianthomas_uk

he/him

English

Brighton, UK

commented 26 January 2014 at 12:37

Issue tags:

+@deprecated

Comment #4

berdir

German

Switzerland

commented 26 January 2014 at 20:16

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.php
@@ -97,17 +97,45 @@ function testNodeAccess() {
-    drupal_static_reset('node_access');
-    variable_set('node_access_test_secret_catalan', 1);
+    \Drupal::entityManager()->getAccessController('node')->resetCache();
+    \Drupal::state()->set('node_access_test_secret_catalan', 1);

Looks all fine to me, tried it locally and the resetCache() call didn't seem to have an effect, it passed without that too, in both cases.

Just wondering if that's a problem, or if we're just doing it because it would otherwise also pass without being tested.

Not sure if it would make sense or if it's even possible to have a call that would fail when the static cache is still in place?