Use the second parameter of \Drupal::state()->get() instead of checking the method didn't return any value

I've review it and tested. The patch applies fine and works great.

This is the kind of thing that should be done consistently for the entirety of core as the per module issue scope is not the best. You can use a regex like [:>]state(\(\))?->get\(.*\? to find more. Note not all should be converted. For example, \Drupal::state()->get(static::class) ?: parent::getAll() should not be convert because you don't always want to call parent::getAll()

I've created a patch based on @alexpott suggestions. Please review.

New patch, which accounts for CssCollectionRendererUnitTest.php expecting NULL.

@Elaman CssCollectionRendererUnitTest is failing because it is not mocking correctly. We can fix it here.

Also there were a few left over in core that can be converted.

@alexplott I thought fixing CssCollectionRendererUnitTest might be a scope-creep.

@Elaman I don't think so - the mock is incorrect and needs adjusting to work how the function actually works.

I can't reproduce the test fail locally. Retesting.

@kiamlaluno yeah but I didn't get that when running the test locally and now DrupalCI is not reporting it either. It's an unrelated random.

• Went through the entire patch, every single change looks correct
• Applied the patch locally, it didn't 100% apply anymore, so I rerolled
• I tried to find more instances of the pattern but this seems still be all of them.

+++ b/core/tests/Drupal/Tests/Core/Asset/CssCollectionRendererUnitTest.php
@@ -26,19 +27,13 @@ class CssCollectionRendererUnitTest extends UnitTestCase {
 
-  /**
-   * The state mock class.
-   *
-   * @var \Drupal\Core\State\StateInterface|\PHPUnit_Framework_MockObject_MockObject
-   */
-  protected $state;
-
   protected function setUp() {
     parent::setUp();
 
-    $this->state = $this->getMock('Drupal\Core\State\StateInterface');
+    $state = $this->prophesize(StateInterface::class);
+    $state->get('system.css_js_query_string', '0')->willReturn('0');
 
-    $this->renderer = new CssCollectionRenderer($this->state);
+    $this->renderer = new CssCollectionRenderer($state->reveal());

That's much nicer!

Forget to RTBC it.

Hi @kiamlaluno.
I found that your patch added changes to method loadEntityByUuid()

- $this->assertFalse(\Drupal::service('entity.repository')->loadEntityByUuid('field_storage_config', $field_storage->uuid()), 'The telephone field has been deleted by the configuration synchronization');
-    $deleted_storages = \Drupal::state()->get('field.storage.deleted') ?: [];
+    $this->assertFalse(\Drupal::entityManager()->loadEntityByUuid('field_storage_config', $field_storage->uuid()), 'The telephone field has been deleted by the configuration synchronization');
+    $deleted_storages = \Drupal::state()->get('field.storage.deleted', []);

I've just updated the latest patch to not apply changes to this part \Drupal::service('entity.repository')->loadEntityByUuid().
Added interdiff file.

Patch from #30 could not be applied to 8.9.x-dev.

Working on this.

Here I have added reroll of patch #30.

There were 6 conflicts in the patch from #30.

Four of these were resolved easily. One concerned a deleted module (path_test).

The changes in CssCollectionRendererUnitTest (6th conflict) were rejected because they are not related to \Drupal::state()->get().

It seems the issue mentioned in #16 to #20 is fixed:

www-data@0ab40c65f6d4:/app/core$ ../vendor/bin/phpunit -c /app/core/phpunit.xml tests/Drupal/Tests/Core/Asset/CssCollectionRendererUnitTest.php
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Asset\CssCollectionRendererUnitTest
.....                                                               5 / 5 (100%)

Time: 319 ms, Memory: 6.00MB

OK (5 tests, 11 assertions)

New patch and diff file attached.

@tvb I like this. But given it's a task the most likely branch for this to land in is 9.1.x - and the so let's roll the next patch against that branch.

Rerolled patch from #36

I reviewed the patch and all changes look correct to me, and of course tests are passing. Applied the patch and used the regex from $13, which found 4 occurrences, none of which should be changed.

 egrep -r '[:>]state(\(\))?->get\(.*\?' core | nl
     1  core/modules/config/tests/config_override_integration_test/src/Cache/ConfigOverrideIntegrationTestCacheContext.php:    $state = \Drupal::state()->get('config_override_integration_test.enabled', FALSE) ? 'yes' : 'no';
     2  core/modules/media/tests/modules/media_test_oembed/src/ProviderRepository.php:    return \Drupal::state()->get(static::class) ?: parent::getAll();
     3  core/modules/basic_auth/tests/modules/basic_auth_test/src/BasicAuthTestController.php:      '#markup' => \Drupal::state()->get('basic_auth_test.state.controller_executed') ? 'yep' : 'nope',
     4  core/modules/block/tests/modules/block_test/src/Plugin/Block/TestAccessBlock.php:    return $this->state->get('test_block_access', FALSE) ? AccessResult::allowed()->setCacheMaxAge(0) : AccessResult::forbidden()->setCacheMaxAge(0);

@quietone, Thanks for reviewing!

rerolled a patch for 8.8.x in case it's needed.

Committed 6b54e95 and pushed to 9.1.x. Thanks!

As a task this is only eligible for 9.1.x