NEXT-13918 - Fix deprecations in migration, storefront and integration suites

keulinho · keulinho · commit 7251237087be · 2022-11-02T11:59:23.000+01:00
diff --git a/.gitlab/CODEOWNERS b/.gitlab/CODEOWNERS
@@ -27,6 +27,7 @@ ecs.php                                                         @product/enginee
 phpstan.neon                                                    @product/engineering/platform-group/core
 phpunit.xml.dist                                                @product/engineering/platform-group/core
 .bc-exclude.php                                                 @product/engineering/platform-group/core
+deprecation.ignore                                              @product/engineering/platform-group/core
 /src/Core/Framework/Resources/config/packages/shopware.yaml     @product/engineering/platform-group/core
 /src/Core/Framework/Resources/**/feature.yaml
 # shared code ownership of multiple groups seems only to work for the last entries
diff --git a/.gitlab/stages/20-unit.yml b/.gitlab/stages/20-unit.yml
@@ -288,6 +288,8 @@ PHP Full:
         - >
             if [ "$PREFER_LOWEST" == "true" ]; then
                 composer update --prefer-lowest --optimize-autoloader
+                # disable symfony deprecation check, when running with lowest dependency versions
+                export SYMFONY_DEPRECATIONS_HELPER=disabled
             fi
         - >
             if [ "$ONLY_CORE" == "true" ]; then
diff --git a/adr/testing/2022-10-20-deprecation-handling-during-phpunit-test-execution.md b/adr/testing/2022-10-20-deprecation-handling-during-phpunit-test-execution.md
@@ -2,21 +2,21 @@
 
 ## Context 
 
-To stay innovative and on the bleeding edge of technology it is important, that to don't rely on deprecated functionality, as it prevents you from using the latest and greatest versions of libraries, that may include important bug fixes, performance improvements or new features.
+To stay innovative and on the bleeding edge of technology it is important, that we don't rely on deprecated functionality, as it prevents us from using the latest and greatest versions of libraries, that may include important bug fixes, performance improvements or new features.
 Relying on deprecated functionality makes continuously upgrading the dependencies harder, as you have a lot of work to remove the deprecated usages before being able to do the upgrades.
-This is not just important for external dependencies, but also for internal deprecations, as when we still rely on some deprecated functionality it makes removing the deprecation very hard.
+This is not just important for external dependencies, but also for internal deprecations, that is we still rely on some deprecated functionality it makes removing the deprecation very hard.
 
 One opportunity is to rely on our PHPUnit test suite to detect usages of deprecated functionality, so we can continuously remove them as they appear and ensure that the code base is always forward compatible.
 
 ## Solution
 
 Because the handling for internal and external deprecations is quite different we probably need different solutions for those cases.
-Especially as for internal deprecations we still want to ensure that they continue working and that those deprecated code paths are also covered by tests.
+Especially as for internal deprecations we still want to ensure that they continue to work and that those deprecated code paths are also covered by tests.
 
 ### Using Symfony's Deprecation Helper for external deprecations
 
 Symfony offers a tool to report all deprecations that are encountered when running the test inside their [PHPUnit Bridge](https://symfony.com/doc/current/components/phpunit_bridge.html).
-With enabling the [`SYMFONY_DEPRECATIONS_HELPER`](https://symfony.com/doc/current/components/phpunit_bridge.html#trigger-deprecation-notices) fo our testsuite we can ensure that no deprecations are triggered while executing the tests.
+With enabling the [`SYMFONY_DEPRECATIONS_HELPER`](https://symfony.com/doc/current/components/phpunit_bridge.html#trigger-deprecation-notices) for our testsuite we can ensure that no deprecations are triggered while executing the tests.
 Previously we could not enable this as it also reported all deprecation usages for internal deprecations and also reported on deprecations that were triggered from inside external dependencies that we could not fix from inside shopware.
 
 But since lately a feature was added to use a [`ignoreFile`](https://symfony.com/doc/current/components/phpunit_bridge.html#ignoring-deprecations), in order to ignore specific deprecations by regex.
@@ -28,9 +28,9 @@ We leverage this feature by using it in a way to ignore all deprecations that we
 
 ### Using our Feature Flag system for internal deprecations
 
-Internally we use the feature flag system to trigger deprecations messages, or throw exceptions if the major feature flag is activated as explained in the [deprecation handling ADR](../workflow/2022-02-28-consistent-deprecation-notices-in-core.md).
+Internally we use the feature flag system to trigger deprecation messages, or throw exceptions if the major feature flag is activated as explained in the [deprecation handling ADR](../workflow/2022-02-28-consistent-deprecation-notices-in-core.md).
 We already use that system in our new unit test suite with a custom `@ActiveFeatures()` annotations, that allows us to run single test cases with a specific set of feature flags.
-But the current implementation has the big drawback that feature flags have to actively be enabled, this leads to following problems:
+But the current implementation has the big drawback that feature flags have to be actively enabled, this leads to following problems:
 1. There are already tests that are not passing after all deprecations are removed, because they rely on deprecated behaviour.
 2. We can't check automatically that our implementation is forward compatible, as the default way of executing tests is without any major flag activated.
 3. It is hard to directly see which test cases are there only to cover legacy/deprecated functionality and can safely be removed after the deprecations are removed.
@@ -39,5 +39,5 @@ Therefore, the workflow is updated in the following way:
 1. All unit tests get executed with all major feature flags activated.
 2. The `@ActiveFeatures()` will be removed, and we introduce a `@DisableFeatures` annotation, that works in the exact opposite way => disabling all feature flags that are passed.
 
-This has the upside that now the default behaviour of our test suite is the new/not-deprecated behaviour, and the deprecated code paths are treated as teh exceptional case instead the other way around.
+This has the upside that now the default behaviour of our test suite is the new/not-deprecated behaviour, and the deprecated code paths are treated as the exceptional case instead the other way around.
 Additionally, all tests that are relying on deprecated behaviour are marked with the `@DisableFeatures` annotations, so it is easy to detect them and simply remove them, if the underlying deprecation was removed.
diff --git a/composer.json b/composer.json
@@ -158,7 +158,7 @@
         "brianium/paratest": "^6.2",
         "dg/bypass-finals": "^1.3",
         "dms/phpunit-arraysubset-asserts": "^0.2.1",
-        "fakerphp/faker": "~1.14.1",
+        "fakerphp/faker": "^1.20",
         "johnkary/phpunit-speedtrap": "~4.0.0",
         "league/construct-finder": "^1.1",
         "maltyxx/images-generator": "~1.0.0",
diff --git a/deprecation-baseline.json b/deprecation-baseline.json
diff --git a/deprecation.ignore b/deprecation.ignore
@@ -15,3 +15,5 @@
 %mb_strtolower\(\)\: Passing null to parameter #1 \(\$string\) of type string is deprecated%
 # Triggered by MessengerEnqueueTransport, which we will remove in 6.5
 %Enqueue\\MessengerAdapter\\EnvelopeItem\\TransportConfiguration%
+# should be fixed with NEXT-23859
+%Returning a file to import for CSS or external references in custom importer callables is deprecated%
diff --git a/phpunit.xml.dist b/phpunit.xml.dist
@@ -86,8 +86,8 @@
         <server name="HTTPS" value="off"/>
         <!--To see the full stackTrace of a Deprecation set the value to a regex matching the deprecation warning-->
         <!--https://symfony.com/doc/current/components/phpunit_bridge.html#display-the-full-stack-trace-->
-        <env name="SYMFONY_DEPRECATIONS_HELPER" value="baselineFile=./deprecation-baseline.json&amp;ignoreFile=./deprecation.ignore" />
-<!--         <env name="SYMFONY_DEPRECATIONS_HELPER" value="/fetchFirstColumn/" />-->
+<!--        <env name="SYMFONY_DEPRECATIONS_HELPER" value="ignoreFile=./deprecation.ignore" />-->
+         <env name="SYMFONY_DEPRECATIONS_HELPER" value="disabled" />
     </php>
     <testsuites>
         <testsuite name="administration">
diff --git a/src/Core/Checkout/Customer/Api/CustomerGroupRegistrationActionController.php b/src/Core/Checkout/Customer/Api/CustomerGroupRegistrationActionController.php
@@ -162,10 +162,6 @@ private function getRequestCustomerIds(Request $request): array
 
         $requestCustomerIds = $request->request->all('customerIds');
 
-        if (\is_string($requestCustomerIds)) {
-            $requestCustomerIds = \json_decode($requestCustomerIds, true);
-        }
-
         if (!empty($requestCustomerIds)) {
             $customerIds = array_unique(array_merge($customerIds, $requestCustomerIds));
         }
diff --git a/src/Core/Content/Product/SearchKeyword/ProductSearchKeywordAnalyzer.php b/src/Core/Content/Product/SearchKeyword/ProductSearchKeywordAnalyzer.php
@@ -13,15 +13,9 @@ class ProductSearchKeywordAnalyzer implements ProductSearchKeywordAnalyzerInterf
 {
     private const MAXIMUM_KEYWORD_LENGTH = 500;
 
-    /**
-     * @var TokenizerInterface
-     */
-    private $tokenizer;
+    private TokenizerInterface $tokenizer;
 
-    /**
-     * @var AbstractTokenFilter
-     */
-    private $tokenFilter;
+    private AbstractTokenFilter $tokenFilter;
 
     /**
      * @internal
@@ -47,24 +41,29 @@ public function analyze(ProductEntity $product, Context $context, array $configF
             $values = array_filter($this->resolveEntityValue($product, $path));
 
             if ($isTokenize) {
-                try {
-                    $values = $this->tokenize($values, $context);
-                } catch (\Throwable $error) {
-                    // Can occur if the resolved value is a nested array. This prevents the implode() from being executed. We ignore this error at this point to allow some error tolerance in the configuration
+                $nonScalarValues = array_filter($values, function ($value) {
+                    return !\is_scalar($value);
+                });
+
+                if ($nonScalarValues !== []) {
                     continue;
                 }
+
+                /** @var array<int, string> $onlyScalarValues */
+                $onlyScalarValues = $values;
+                $values = $this->tokenize($onlyScalarValues, $context);
             }
 
             foreach ($values as $value) {
-                try {
-                    // even the field is non tokenize, if it reached 500 chars, we should break it anyway
-                    $parts = array_filter(mb_str_split((string) $value, self::MAXIMUM_KEYWORD_LENGTH));
+                if (!\is_scalar($value)) {
+                    continue;
+                }
 
-                    foreach ($parts as $part) {
-                        $keywords->add(new AnalyzedKeyword((string) $part, $ranking));
-                    }
-                } catch (\Throwable $error) {
-                    // Can occur if the resolved value is a nested array. This prevents the string cast from being executed (Array to string conversion). We ignore this error at this point to allow some error tolerance in the configuration
+                // even the field is non tokenize, if it reached 500 chars, we should break it anyway
+                $parts = array_filter(mb_str_split((string) $value, self::MAXIMUM_KEYWORD_LENGTH));
+
+                foreach ($parts as $part) {
+                    $keywords->add(new AnalyzedKeyword((string) $part, $ranking));
                 }
             }
         }
@@ -87,7 +86,7 @@ private function tokenize(array $values, Context $context): array
     }
 
     /**
-     * @return array<int, string>
+     * @return array<int, string|array<mixed>>
      */
     private function resolveEntityValue(Entity $entity, string $path): array
     {
diff --git a/src/Core/Content/Test/Media/Api/MediaUploadControllerTest.php b/src/Core/Content/Test/Media/Api/MediaUploadControllerTest.php
@@ -269,7 +269,6 @@ private function getMediaEntity(): MediaEntity
 
         static::assertSame(Response::HTTP_NO_CONTENT, $response->getStatusCode(), (string) $response->getContent());
 
-
         $location = $response->headers->get('Location');
         static::assertIsString($location);
         static::assertStringEndsWith(
diff --git a/src/Core/Migration/Test/Migration1619604605FixListingPricesUsageTest.php b/src/Core/Migration/Test/Migration1619604605FixListingPricesUsageTest.php
@@ -308,7 +308,7 @@ private function createCmsPage(?array $sorting = null): array
 
         $page = [
             'id' => Uuid::randomHex(),
-            'name' => $faker->company,
+            'name' => $faker->company(),
             'type' => 'landing_page',
             'sections' => [
                 [
diff --git a/src/Core/System/Snippet/SnippetService.php b/src/Core/System/Snippet/SnippetService.php
@@ -291,9 +291,12 @@ protected function getUnusedThemes(): array
      */
     protected function fetchSnippetsFromDatabase(string $snippetSetId): array
     {
-        return $this->connection->fetchAllKeyValue('SELECT translation_key, value FROM snippet WHERE snippet_set_id = :snippetSetId', [
+        /** @var array<string, string> $snippets */
+        $snippets = $this->connection->fetchAllKeyValue('SELECT translation_key, value FROM snippet WHERE snippet_set_id = :snippetSetId', [
             'snippetSetId' => Uuid::fromHexToBytes($snippetSetId),
         ]);
+
+        return $snippets;
     }
 
     /**
diff --git a/src/Core/System/Test/Snippet/Mock/MockSnippetFile.php b/src/Core/System/Test/Snippet/Mock/MockSnippetFile.php
@@ -26,6 +26,13 @@ public function __construct(string $name, ?string $iso = null, string $content =
         $this->technicalName = $technicalName;
     }
 
+    public static function cleanup(): void
+    {
+        foreach (glob(__DIR__ . '/_fixtures/*.json') ?: [] as $mockFile) {
+            unlink($mockFile);
+        }
+    }
+
     public function getName(): string
     {
         return $this->name;
diff --git a/src/Core/Test/FeatureFlagExtension.php b/src/Core/Test/FeatureFlagExtension.php
@@ -31,7 +31,7 @@ class FeatureFlagExtension implements BeforeTestHook, AfterTestHook
 
     private bool $testMode;
 
-    public function __construct(string $namespacePrefix = 'Shopware\\Tests\\Unit\\', bool $testMode = false)
+    public function __construct(string $namespacePrefix = 'Shopware\\Tests\\', bool $testMode = false)
     {
         $this->annotationReader = new AnnotationReader();
         $this->namespacePrefix = $namespacePrefix;
diff --git a/src/Core/composer.json b/src/Core/composer.json
@@ -142,7 +142,7 @@
         "ext-xmlwriter": "*",
         "bheller/images-generator": "~1.0.1",
         "dms/phpunit-arraysubset-asserts": "^0.2.1",
-        "fzaninotto/faker": "~1.9.1",
+        "fakerphp/faker": "^1.20",
         "johnkary/phpunit-speedtrap": "~4.0.0",
         "mbezhanov/faker-provider-collection": "~2.0.1",
         "nikic/php-parser": "~4.13.2",
diff --git a/tests/integration/php/Core/Content/Media/File/FileFetcherTest.php b/tests/integration/php/Core/Content/Media/File/FileFetcherTest.php
@@ -22,6 +22,7 @@
 class FileFetcherTest extends TestCase
 {
     public const TEST_IMAGE = __DIR__ . '/../../../../../../../src/Core/Content/Test/Media/fixtures/shopware-logo.png';
+
     private FileFetcher $fileFetcher;
 
     private bool $mediaDirCreated = false;
diff --git a/tests/integration/php/Core/System/Snippet/SnippetServiceTest.php b/tests/integration/php/Core/System/Snippet/SnippetServiceTest.php
@@ -26,9 +26,7 @@ class SnippetServiceTest extends TestCase
 
     public function tearDown(): void
     {
-        foreach (glob(__DIR__ . '/Mock/_fixtures/*.json') ?: [] as $mockFile) {
-            unlink($mockFile);
-        }
+        MockSnippetFile::cleanup();
     }
 
     public function getStorefrontSnippetsForNotExistingSnippetSet(): void
diff --git a/tests/integration/php/Elasticsearch/Product/ElasticsearchProductTest.php b/tests/integration/php/Elasticsearch/Product/ElasticsearchProductTest.php
@@ -70,7 +70,6 @@
 use Shopware\Core\System\SalesChannel\Context\SalesChannelContextFactory;
 use Shopware\Core\System\SalesChannel\Context\SalesChannelContextService;
 use Shopware\Core\System\SalesChannel\SalesChannelContext;
-use Shopware\Core\Test\Annotation\ActiveFeatures;
 use Shopware\Core\Test\TestDefaults;
 use Shopware\Elasticsearch\Framework\DataAbstractionLayer\ElasticsearchEntityAggregator;
 use Shopware\Elasticsearch\Framework\DataAbstractionLayer\ElasticsearchEntitySearcher;
@@ -83,8 +82,6 @@
 use function array_combine;
 
 /**
- * @ActiveFeatures(features={})
- *
  * @internal
  * @group skip-paratest
  */
@@ -209,6 +206,8 @@ public function testIndexing(): IdsCollection
 
             $this->clearElasticsearch();
 
+            $this->resetStopWords();
+
             $this->ids->set('currency', $this->currencyId);
             $this->ids->set('anotherCurrency', $this->anotherCurrencyId);
             $currencies = [
@@ -3760,4 +3759,14 @@ private function createIndexingContext(): Context
 
         return $context;
     }
+
+    /**
+     * Some tests use terms that are excluded by the default configuration in the administration.
+     * Therefore we reset the configuration.
+     */
+    private function resetStopWords(): void
+    {
+        $connection = $this->getContainer()->get(Connection::class);
+        $connection->executeStatement('UPDATE `product_search_config` SET `excluded_terms` = "[]"');
+    }
 }
diff --git a/tests/integration/php/Elasticsearch/Product/ProductSearchQueryBuilderTest.php b/tests/integration/php/Elasticsearch/Product/ProductSearchQueryBuilderTest.php
@@ -22,15 +22,12 @@
 use Shopware\Core\Framework\Test\TestDataCollection;
 use Shopware\Core\Framework\Uuid\Uuid;
 use Shopware\Core\System\CustomField\CustomFieldTypes;
-use Shopware\Core\Test\Annotation\ActiveFeatures;
 use Shopware\Elasticsearch\Product\ElasticsearchProductDefinition;
 use Shopware\Elasticsearch\Product\Event\ElasticsearchProductCustomFieldsMappingEvent;
 use Shopware\Elasticsearch\Test\ElasticsearchTestTestBehaviour;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**
- * @ActiveFeatures(features={"FEATURE_NEXT_22900"})
- *
  * @internal
  *
  * @covers \Shopware\Elasticsearch\Product\ProductSearchQueryBuilder
diff --git a/tests/migration/Core/V6_3/Migration1582011195FixCountryStateGermanTranslationTest.php b/tests/migration/Core/V6_3/Migration1582011195FixCountryStateGermanTranslationTest.php
@@ -3,8 +3,8 @@
 namespace Shopware\Tests\Migration\Core\V6_3;
 
 use Doctrine\DBAL\Connection;
+use Doctrine\DBAL\ForwardCompatibility\Result;
 use Doctrine\DBAL\Query\QueryBuilder;
-use Doctrine\DBAL\Statement;
 use PHPUnit\Framework\TestCase;
 use Shopware\Core\Framework\Test\TestCaseBase\KernelLifecycleManager;
 use Shopware\Core\Migration\V6_3\Migration1582011195FixCountryStateGermanTranslation;
@@ -18,8 +18,8 @@ class Migration1582011195FixCountryStateGermanTranslationTest extends TestCase
     public function testNoGermanLanguageId(): void
     {
         $connectionMock = $this->createMock(Connection::class);
-        $resultStatement = $this->createMock(Statement::class);
-        $resultStatement->method('fetch')->willReturn(false);
+        $resultStatement = $this->createMock(Result::class);
+        $resultStatement->method('fetchOne')->willReturn(false);
 
         // early return because of no german language id by locale de-DE
         $resultStatement->expects(static::never())->method('fetchAllAssociative');
diff --git a/tests/unit/php/Core/Content/Product/SearchKeyword/ProductSearchKeywordAnalyzerTest.php b/tests/unit/php/Core/Content/Product/SearchKeyword/ProductSearchKeywordAnalyzerTest.php
@@ -42,7 +42,7 @@ public function testAnalyze(array $productData, array $configFields, array $expe
 
         $tokenizer = new Tokenizer(3);
         $tokenFilter = $this->createMock(TokenFilter::class);
-        $tokenFilter->method('filter')->willReturnCallback(function (array $tokens, Context $context) {
+        $tokenFilter->method('filter')->willReturnCallback(function (array $tokens) {
             return $tokens;
         });
 
@@ -56,6 +56,25 @@ public function testAnalyze(array $productData, array $configFields, array $expe
         static::assertEquals($expected, $analyzerResult);
     }
 
+    /**
+     * The old implementation relied on the error_reporting level, to also report notices as errors.
+     * This test ensures that the new implementation does not rely on the error_reporting level.
+     *
+     * @dataProvider analyzeCases
+     *
+     * @param array<string, mixed> $productData
+     * @param array<int, array{field: string, tokenize: bool, ranking: int}> $configFields
+     * @param array<int, string> $expected
+     */
+    public function testAnalyzeWithIgnoredErrorNoticeReporting(array $productData, array $configFields, array $expected): void
+    {
+        $oldLevel = error_reporting(\E_ERROR);
+
+        $this->testAnalyze($productData, $configFields, $expected);
+
+        error_reporting($oldLevel);
+    }
+
     /**
      * @return iterable<string, array{0:array<string, array<string, string|array<int|string, string|array<int|string>>>|int|string|TagCollection>, 1:array<int, array{field: string, tokenize: bool, ranking: int}>, 2:array<int, int|string>}>
      */
diff --git a/tests/unit/php/Core/System/Snippet/Files/SnippetFileLoaderTest.php b/tests/unit/php/Core/System/Snippet/Files/SnippetFileLoaderTest.php

Original file line number	Diff line number	Diff line change
`@@ -162,10 +162,6 @@ private function getRequestCustomerIds(Request $request): array`
`162`	`162`
`163`	`163`	`$requestCustomerIds = $request->request->all('customerIds');`
`164`	`164`
`165`		`- if (\is_string($requestCustomerIds)) {`
`166`		`- $requestCustomerIds = \json_decode($requestCustomerIds, true);`
`167`		`- }`
`168`		`-`
`169`	`165`	`if (!empty($requestCustomerIds)) {`
`170`	`166`	`$customerIds = array_unique(array_merge($customerIds, $requestCustomerIds));`
`171`	`167`	`}`