After submitting a patch for #3125842: Block facets don't apply contextual filters on ajax response I noticed that the default tests appear to be failing using PHP 7.3 & MySQL 5.6, Drupal 8.9.16. The most recent tests on 8.x-1.x-dev did pass but that was on 6 Oct 2021.

One example:

1) Drupal\Tests\facets\FunctionalJavascript\WidgetJSTest::testAddFacet
Drupal\Core\Entity\EntityStorageException: Exception thrown while performing a schema update. SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 767 bytes: CREATE TABLE {search_api_task} (
`id` INT unsigned NOT NULL auto_increment, 
`type` VARCHAR(50) NULL DEFAULT NULL, 
`server_id` VARCHAR(50) NULL DEFAULT NULL, 
`index_id` VARCHAR(50) NULL DEFAULT NULL, 
`data` LONGTEXT NULL DEFAULT NULL, 
PRIMARY KEY (`id`), 
UNIQUE KEY `task__unique` (`type`, `server_id`, `index_id`, `data`(255))
) ENGINE = InnoDB DEFAULT CHARACTER SET utf8mb4 COMMENT 'The base table for search_api_task entities.'; Array
(
)


/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1619
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1538
/var/www/html/core/lib/Drupal/Core/Entity/EntityTypeListener.php:93
/var/www/html/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php:232
/var/www/html/core/lib/Drupal/Core/Extension/ModuleInstaller.php:254
/var/www/html/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/var/www/html/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:476
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:83
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:578
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:406
/var/www/html/modules/contrib/facets/tests/src/FunctionalJavascript/JsBase.php:34

Caused by
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 767 bytes: CREATE TABLE {search_api_task} (
`id` INT unsigned NOT NULL auto_increment, 
`type` VARCHAR(50) NULL DEFAULT NULL, 
`server_id` VARCHAR(50) NULL DEFAULT NULL, 
`index_id` VARCHAR(50) NULL DEFAULT NULL, 
`data` LONGTEXT NULL DEFAULT NULL, 
PRIMARY KEY (`id`), 
UNIQUE KEY `task__unique` (`type`, `server_id`, `index_id`, `data`(255))
) ENGINE = InnoDB DEFAULT CHARACTER SET utf8mb4 COMMENT 'The base table for search_api_task entities.'; Array
(
)


/var/www/html/core/lib/Drupal/Core/Database/Connection.php:701
/var/www/html/core/lib/Drupal/Core/Database/Connection.php:665
/var/www/html/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php:357
/var/www/html/core/lib/Drupal/Core/Database/Schema.php:656
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:432
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1537
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1611
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1538
/var/www/html/core/lib/Drupal/Core/Entity/EntityTypeListener.php:93
/var/www/html/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php:232
/var/www/html/core/lib/Drupal/Core/Extension/ModuleInstaller.php:254
/var/www/html/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/var/www/html/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:476
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:83
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:578
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:406
/var/www/html/modules/contrib/facets/tests/src/FunctionalJavascript/JsBase.php:34

Caused by
PDOException: SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 767 bytes

/var/www/html/core/lib/Drupal/Core/Database/Statement.php:59
/var/www/html/core/lib/Drupal/Core/Database/Connection.php:640
/var/www/html/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php:357
/var/www/html/core/lib/Drupal/Core/Database/Schema.php:656
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:432
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1537
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1611
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1538
/var/www/html/core/lib/Drupal/Core/Entity/EntityTypeListener.php:93
/var/www/html/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php:232
/var/www/html/core/lib/Drupal/Core/Extension/ModuleInstaller.php:254
/var/www/html/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/var/www/html/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:476
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:83
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:578
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:406
/var/www/html/modules/contrib/facets/tests/src/FunctionalJavascript/JsBase.php:34

It seems that the issue stems somehow from Search API which had a 8.x-1.21 release on 4 Nov that included #3228210: Fix current test failures. The weird thing is that issue was reporting a similar error but should have fixed it so I'm not quite sure why this is now causing test failures here.

CommentFileSizeAuthor
#10 3248297-9.patch14.22 KBwells
#10 interdiff_8-9.txt1.46 KBwells
#8 3248297-8.patch14.27 KBwells
#8 interdiff_7-8.txt2.64 KBwells
#7 3248297-7.patch11.1 KBwells
#7 interdiff_6-7.txt1.13 KBwells
#6 3248297-6.patch9.73 KBwells
#6 interdiff_5-6.txt1.33 KBwells
#5 3248297-5.patch8.27 KBwells
#5 interdiff_4-5.txt900 byteswells
#4 3248297-4.patch7.14 KBwells
#2 3248297-2.patch338 byteswells

Issue fork facets-3248297

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

wells created an issue. See original summary.

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 3248297-2.patch338 bytes

Adding a patch with a small, trivial change just to trigger tests. Assuming this work there should be a result similar 8.x-1.x: PHP 7.3 & MySQL 5.6, D8.9.16 309 pass, 34 fail.

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented

Based on the discussion in #3228210: Fix current test failures it seems that Search API is is just no longer compatible with MySQL 5.6 as of 8.x-1.21. That seems reasonable given that Drupal 8 is now EOL and Drupal 9+ requires MySQL 5.7.

I ran the PHP 7.3 & MySQL 5.7, Drupal 8.9.x tests and they all pass fine so maybe Facets should update to use those as the default for commits and issues?

Incidentally, I also ran the PHP 7.3 & MySQL 5.7, Drupal 9.3.x tests and that has a number of failures unrelated to this specific issue. I think it would be worth using this issue to get those fixed up and either set that as the default tests or include it in regular tests.

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 3248297-4.patch7.14 KB

Knocking out an easy one first -- Zend\Stdlib\ArrayObject is no longer available in D9+ so changing all instances to the SPL ArrayObject (as others tests do).

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented
StatusFileSize
new interdiff_4-5.txt900 bytes
new 3248297-5.patch8.27 KB

Four additional issues to address from the last test:

Drupal\Tests\facets_summary\Functional\IntegrationTest::testBlockDelete
Behat\Mink\Exception\ResponseTextException: The text "Owl" was not found anywhere in the text of the current page.

Fixed. This was caused by \Drupal\Tests\facets_summary\Functional\IntegrationTest::testBlockDelete referencing the classy theme when it is no longer the default theme in tests (I think...). I updated that test to reference \Drupal\Tests\facets\Functional\FacetsTestBase::$defaultTheme instead of using a hardcoded string. I think this will resolve the issue in D9 tests without affecting D8 tests.

Drupal\Tests\facets\Functional\UrlIntegrationTest::testUrlIntegration
Behat\Mink\Exception\ExpectationException: Current page is "/search-api-test-fulltext?y%5B0%5D=facet%3Aitem", but "/search-api-test-fulltext?y%5B0%5D=facet%7C%7Citem" expected.

Fixed by #7. Not sure about this one yet. The expected value is /search-api-test-fulltext?y[0]=facet||item and actual is /search-api-test-fulltext?y[0]=facet:item. I'm not seeing the double-pipes tested anywhere else and I'm unclear on what they are for. This is somehow related to \Drupal\facets_query_processor\Plugin\facets\url_processor\DummyQuery but, again, it's unclear what that is or what it is testing.

Drupal\Tests\facets\Functional\UrlIntegrationTest::testFacetUrlCanBeChanged
Behat\Mink\Exception\ExpectationException: Current page is "/search-api-test-fulltext?test=fun&f%5B0%5D=facet%3Aitem", but "/search-api-test-fulltext?f%5B0%5D=facet%3Aitem&test=fun" expected.

Unfixed. Ditto unsure here. The expected value is /search-api-test-fulltext?f[0]=facet:item&test=fun and the actual is /search-api-test-fulltext?test=fun&f[0]=facet:item. So the order of the query parameters are just switched. I don't think that should matter so maybe a change can be made to the assertion to support either result?

Drupal\Tests\facets\Functional\IntegrationTest::testFramework
Exception: TypeError: Return value of Drupal\Core\Entity\EntityStorageBase::getEntityClass() must be of the type string, null returned
Drupal\Core\Entity\EntityStorageBase->getEntityClass()() (Line: 115)

Fixed by #3248533: Attempting to create a facet with a duplicate machine name causes a PHP error in Drupal 9.

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented
StatusFileSize
new interdiff_5-6.txt1.33 KB
new 3248297-6.patch9.73 KB

One additional issue discovered in #3248533: Attempting to create a facet with a duplicate machine name causes a PHP error in Drupal 9 -- after that patch is merged tests will also hit an error in \Drupal\Tests\facets\Functional\IntegrationTest::testFramework due to a change in the success message when deleting a block which is asserted in Drupal\Tests\facets\Functional\BlockTestTrait::deleteBlock. This change was made by #2839558: Blocks do not have a "remove" contextual link in Drupal 9.3.

Attached patch removes the old logic for this message that addressed Drupal 8.3 (since the module now only supports 8.8+) and replaces it with logic for the Drupal 9.3 change. I won't run tests on this patch as it won't show any difference here until #3248533: Attempting to create a facet with a duplicate machine name causes a PHP error in Drupal 9 is merged.

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented
StatusFileSize
new interdiff_6-7.txt1.13 KB
new 3248297-7.patch11.1 KB

Next patch attached! I think this will fix the second fail from #5. This test was failing because \Drupal\facets_query_processor\Plugin\facets\url_processor\DummyQuery was setting a $separator value but that value was being overwritten by \Drupal\facets\UrlProcessor\UrlProcessorPluginBase::__construct. This patch changes DummyQuery to implement it's separator override in it's own __construct override. The test now passes in D9 but I'm unclear on how this was even passing in D8 so I'm curious to see if this causes a fail in the D8 tests in CI.

So that only leaves the third fail from #5 and I'm entirely unsure whats happening with that one. I can't find any changes to URL or query parameter handling that would have made the order of those parameters switch between D8 and D9 (or their dependencies). I tested manually using the test modules and configuration and sure enough they are switched in D9. I don't have a D8 install to test with but I assume the expected order is correct there because all the D8 tests pass. Supporting either result would require refactoring \Drupal\Tests\facets\Functional\TestHelperTrait::checkClickedFacetUrl and the tests that rely on it so I'm avoiding that for now (though I think it would really just mean moving the asserting outside that method).

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
StatusFileSize
new interdiff_7-8.txt2.64 KB
new 3248297-8.patch14.27 KB

Ok last fix! This patch refactors \Drupal\Tests\facets\Functional\TestHelperTrait::checkClickedFacetUrl as \Drupal\Tests\facets\Functional\TestHelperTrait::clickFacet and only clicks but does not assert any URL. That allows \Drupal\Tests\facets\Functional\UrlIntegrationTest::testFacetUrlCanBeChanged to be changed in such a way that it just tests for the existence of the relevant query parameters on the URL independently instead of relying on an exact URL string.

This should be the last patch needed after #3248533: Attempting to create a facet with a duplicate machine name causes a PHP error in Drupal 9 is merged (i.e., there will still be one failure here).

Moving this to Needs review though it will of course fail and reset to Needs work (:

Status: Needs review » Needs work

The last submitted patch, 8: 3248297-8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented
StatusFileSize
new interdiff_8-9.txt1.46 KB
new 3248297-9.patch14.22 KB

Jinxed myself! This patch removes the toString()'s I added to the relevant URLs in #8 because apparently that causes some unhappiness when testing (see #2986962: BrowserTestBase::drupalGet() does not appear to be handling base url properly). This change should get the patch back to passing in D8 and one fail in D9 (again, pending merge of #3248533: Attempting to create a facet with a duplicate machine name causes a PHP error in Drupal 9).

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Status: Needs work » Needs review

  • mkalkbrenner committed c47b6e0 on 2.0.x authored by wells 
    Issue #3248297 by wells, mkalkbrenner: Default tests failing
mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Version: 8.x-1.x-dev » 2.0.x-dev
Priority: Normal » Major
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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