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.

Files: 
CommentFileSizeAuthor
#5 2175739.5.patch9.13 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 63,364 pass(es). View
#5 1-5-interdiff.txt1.2 KBalexpott
#1 2175739.1.patch8.88 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,935 pass(es). View
#1 2175739.1.will-fail.patch8.81 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,965 pass(es), 2 fail(s), and 0 exception(s). View
#1 interdiff.txt800 bytesalexpott

Comments

alexpott’s picture

Status: Active » Needs review
Issue tags: +Configuration system
FileSize
800 bytes
8.81 KB
FAILED: [[SimpleTest]]: [MySQL] 59,965 pass(es), 2 fail(s), and 0 exception(s). View
8.88 KB
PASSED: [[SimpleTest]]: [MySQL] 59,935 pass(es). View

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.

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

ianthomas_uk’s picture

Issue tags: +@deprecated
Berdir’s picture

+++ 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?

alexpott’s picture

FileSize
1.2 KB
9.13 KB
PASSED: [[SimpleTest]]: [MySQL] 63,364 pass(es). View

Added a test to show that the static cache can be tested and why it needs to be cleared.

alexpott’s picture

Priority: Major » Critical
Issue tags: +beta blocker

This should actually be critical since it is blocking #2167109: Remove Variable subsystem

Gábor Hojtsy’s picture

@Berdir: looks good? :)

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yes, RTBC if testbot agrees.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. ROCK!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.