Feature/7 default display banner navigation

[finnlewis]

No description provided.

[finnlewis]

@andybroomfield @Adnan-cds @stephen-cox @ekes anyone able to make any more suggestions on fixing the remaining errors on these tests? I'm getting there, slowly!

1) Drupal\Tests\localgov_subsites\Kernel\SubsitePagesTest::testUpdateSubsitePages
Drupal\Component\Plugin\Exception\PluginNotFoundException: Unable to determine class for field type 'entity_reference_hierarchy' found in the 'field.storage.0' configuration 

2) Drupal\Tests\localgov_subsites\Functional\SubsiteBlocksTest::testSubsiteBannerBlock
Undefined index: username

[Adnan-cds]

Unable to determine class for field type 'entity_reference_hierarchy' found in the 'field.storage.0' configuration

The entity_reference_hierarchy field is defined in the entity_hierarchy module. So if you please add it to the list of modules needed to run this test, this error should go away.

I haven't checked the other issue about the username :(

[Adnan-cds]

Hi James,
IIRC repository declarations only work in root-level composer.json files. As a quick workaround, you can try updating the SQLite authentication details used in the test runner Docker container. Currently it says sqlite://localhost/dev/shm/test.sqlite. Replacing it with sqlite://database:database@localhost/dev/shm/test.sqlite may work. This will of course mean rebuilding the Docker container which Steve can do later.

I have to say I haven't tested this at all. If you have SQLite in your development site, you should be able to try it there first.

FAO @Iam-Jam @stephen-cox

[j4-m]

Quick summary of my changes:

The entity hierarchy_module depends on dbal. The latter causes undefined index notices when using SQLite (which is being used by the GitHub test runner) and consequently the tests would fail. I was originally trying to include a fix for DBAL via a Composer alias to a fork of the project but this would need to done further up the composer tree so I am now trying to do this via a composer patch in this project.

[j4-m]

Currently the tests are failing due an exception raised by a dependency of the DBAL module see PNX\NestedSet\Storage\BaseDbalStorage::__construct I can re-create this issue locally if I run the tests using SQLite but not when manually testing the site using SQLite.

We do not run into this exception when running the tests with MySQL.

cc @finnlewis

[Adnan-cds]

Is this the code throwing the exception James?

[finnlewis]

That does look like the code generating the error @Adnan-cds - any ideas how we fix it?

[Adnan-cds]

any ideas how we fix it?

I tried to run the Kernel test bundled with dbal in a fresh install of localgov using Sqlite as the database. It indeed fails although the error is slightly different - it can't find a database table. This will take some time to troubleshoot. I may be able to look into it next week. In the mean time, I don't mind approving this pull request if you comment out this Kernel test of localgov_subsites after verifying that it runs okay in MySQL.

[finnlewis]

I cannot reproduce these errors locally.

Errors seen in https://github.com/localgovdrupal/localgov_subsites/pull/8/checks?check_run_id=1489320195

1) Drupal\Tests\localgov_subsites\Functional\SubsiteBlocksTest::testSubsiteBannerBlock
Drupal\Core\Entity\EntityStorageException: Table name must match the regex /^[a-zA-Z]\w{1,64}$/

/var/www/html/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:846
/var/www/html/web/core/lib/Drupal/Core/Entity/EntityBase.php:395
/var/www/html/web/core/modules/node/tests/src/Traits/NodeCreationTrait.php:98
/var/www/html/web/modules/contrib/localgov_subsites/tests/src/Functional/SubsiteBlocksTest.php:127

Caused by
InvalidArgumentException: Table name must match the regex /^[a-zA-Z]\w{1,64}$/

/app/vendor/previousnext/nested-set/src/Storage/BaseDbalStorage.php:42
/app/web/modules/contrib/entity_hierarchy/src/Storage/NestedSetStorage.php:54
/app/web/modules/contrib/entity_hierarchy/src/Storage/NestedSetStorageFactory.php:87
/app/web/modules/contrib/entity_hierarchy/src/Storage/NestedSetStorageFactory.php:71
/app/web/modules/contrib/entity_hierarchy/src/Plugin/Field/FieldType/EntityReferenceHierarchy.php:200
/app/web/modules/contrib/entity_hierarchy/src/Plugin/Field/FieldType/EntityReferenceHierarchy.php:127
/app/web/core/lib/Drupal/Core/Field/FieldItemList.php:233
/app/web/core/lib/Drupal/Core/Field/FieldItemList.php:198
/app/web/modules/contrib/entity_hierarchy/src/Plugin/Field/FieldType/EntityReferenceHierarchyFieldItemList.php:33
/app/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:881
/app/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:913
/app/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:835
/app/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:535
/app/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:728
/app/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:460
/app/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:837
/app/web/core/lib/Drupal/Core/Entity/EntityBase.php:395
/app/web/core/modules/node/tests/src/Traits/NodeCreationTrait.php:98
/app/web/modules/contrib/localgov_subsites/tests/src/Functional/SubsitePagesTest.php:117

Steps to try to reproduce:

1. Create sites/default/settings.local.php with

$databases['default']['default'] = [
  'driver' => 'sqlite',
  'database' => '/app/localgov.sqlite',
];

2. Install drupal localgov profile

3. Enable localgov_subsites

4. Debug line /app/web/modules/contrib/entity_hierarchy/src/Storage/NestedSetStorage.php:54

5. Create subsite_overview and subsite_page

Result:

No errors.

The table name appears to be:

nested_set_localgov_subsites_parent_node

Which seems reasonable - my regex is not good enough to spot where that might fail in /^[a-zA-Z]\w{1,64}$/:

https://github.com/previousnext/nested-set/blob/d471ba1cab04066d33ff7a76ba616f8afb873570/src/Storage/BaseDbalStorage.php#L42

[finnlewis]

@Adnan-cds thanks for looking... all the tests pass for me locally when running in MySQL !!!

[Adnan-cds]

all the tests pass for me locally when running in MySQL !!!

In that case you need to decide how best to preserve the Kernel test class but not run it! Perhaps Steve will have some idea :) Sorry, I am trying to avoid getting involved into anything this week :(

[finnlewis]

@stephen-cox and @ekes any brain power or time to help out with this one?

One thing I am wondering is whether we should / could move to using MySQL to test against in the Github testing workflow.

I am running some tests locally to benchmark both SQLite and MySQL against the same tests to confirm the performance differential.

Other than that, any idea how to fix this SQLite testing issue with dbal ?

[finnlewis]

For the record, running locally with MySQL, the tests do now pass

This is conditional on

1. the included patch for layout_paragraphs to fix the schema
2. the feature/45-responsive-image-28-9 of localgov_core which includes a default image style.

www-data@f55890ac2655:/app$ phpunit -v --debug web/modules/contrib/localgov_subsites/tests/
PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.2.34 with Xdebug 2.9.8
Configuration: /app/phpunit.xml

Testing web/modules/contrib/localgov_subsites/tests/
Test 'Drupal\Tests\localgov_subsites\Functional\SubsiteBlocksTest::testSubsiteBannerBlock' started
Test 'Drupal\Tests\localgov_subsites\Functional\SubsiteBlocksTest::testSubsiteBannerBlock' ended
Test 'Drupal\Tests\localgov_subsites\Functional\SubsiteBlocksTest::testSubsiteNavigationBlock' started
Test 'Drupal\Tests\localgov_subsites\Functional\SubsiteBlocksTest::testSubsiteNavigationBlock' ended
Test 'Drupal\Tests\localgov_subsites\Functional\SubsitePagesTest::testSubsiteFields' started
Test 'Drupal\Tests\localgov_subsites\Functional\SubsitePagesTest::testSubsiteFields' ended
Test 'Drupal\Tests\localgov_subsites\Functional\SubsitePagesTest::testSubsitePaths' started
Test 'Drupal\Tests\localgov_subsites\Functional\SubsitePagesTest::testSubsitePaths' ended


Time: 14.78 minutes, Memory: 6.00 MB

OK (4 tests, 65 assertions)

[andybroomfield]

This is what I have on my old Mac to use sqlite outside lando
<env name="SIMPLETEST_DB" value="sqlite://localhost//tmp/localgovdrupal.db"/>
However I'm starting to see similar errors to @finnlewis now.
There is a known issue with sqlite and row locking.

How slow do the tests run with MySQL on Github? Would it really be a huge cost to use that?

[j4-m]

I have re-created the locally using SQLite. In order to do so I used the following connection config.:

$databases['default']['default'] = [
 'driver' => 'sqlite',
 'database' => '../sqlitedbfiles/.ht.sqlite',
  'prefix' => 'test',
];

With the prefix being key to this issue.

The exception originates in a vendor library which I have hacked locally but this had led to further exceptions from the same library.

We could continue to try to fix these issues, although very unclear how much effort's gonna be involved, but these issues are specific to SQLite using a prefix which I would argue is fairly edge case.

So our options are:

1. Fix the SQLite\prefix specific issues.
2. Use SQLite but don't use a prefix (somehow).
3. Don't use SQLite.

EDIT:

This may be useful:
https://freek.dev/1590-how-to-use-a-mysql-database-on-github-actions

[finnlewis]

Thanks James, SQLite is significantly faster for tests.

It will also take a rebuild of the test runner / docker image to change these from sqlite to MySQL. Possibly worth a try but will require co-ordination from @stephen-cox @Adnan-cds @andybroomfield to make sure we are all aware of and support the changes.

I do think the best solution would be to get the libraries workgin with SQLite.

The maintainer of the dbal module and that library said that he was not aware of the prefix issue on https://www.drupal.org/project/dbal/issues/3186378#comment-13928669

So I reckon he might be able to help is resolve the issues in his library.

If we can articulate the problem and the cause on Github issues, he seems quick to respond. @ekes might be able to help too.

@stephen-cox, @ekes and I are in DrupalCon Sessions all day / week so may be less available than usual.

(Or maybe we'll get bored and dive in!?!)

[Adnan-cds]

@stephen-cox, @ekes and I are in DrupalCon Sessions all day / week so may be less available than usual.

Good morning. I hope you all have a good time there :)

We are expecting to go live today, so I hope to have some breathing space once that happens. Weather is not looking good though (foggy), so launch may be delayed. But I am hopeful. I would like to have a stab at this dbal/sqlite chaos post-launch.

[j4-m]

There is probably one more option: ditching entity_hiearchy....

[j4-m]

Update on where my debugging has taken me:

After hacking the regex in the nested-set library we get a DBAL TableNotFoundException:

Doctrine\DBAL\Exception\TableNotFoundException: An exception occurred while executing 'SELECT parent.id, parent.revision_id, parent.left_pos, parent.right_pos, parent.depth FROM test.nested_set_localgov_subsites_parent_node AS child, test.nested_set_localgov_subsites_parent_node AS parent WHERE child.left_pos BETWEEN parent.left_pos AND parent.right_pos AND child.id = ? AND child.revision_id = ? ORDER BY parent.left_pos' with params ["1", "1"]: SQLSTATE[HY000]: General error: 1 no such table: test.nested_set_localgov_subsites_parent_node in Doctrine\DBAL\Driver\AbstractSQLiteDriver->convertException() (line 63 of /app/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractSQLiteDriver.php).

Using the sqlite3 CLI I could confirm that test.nested_set_localgov_subsites_parent_node does not exist nor is there a table in any of the SQLite databases with the name nested_set_localgov_subsites_parent_node.

The entity_hierachy module attempts to create this table here: https://git.drupalcode.org/project/entity_hierarchy/-/blob/8.x-2.x/src/Storage/NestedSetStorage.php#L116

Which uses the nested-set library to create the Doctrine DBAL schema object and general the create table SQL. At this point, the table name passed to DBAL is still test.nested_set_localgov_subsites_parent_node but DBAL transforms the . to __ https://github.com/doctrine/dbal/blob/2.12.x/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php#L329.

As far as I can tell DBAL does not support creating databases within schemas:

https://github.com/doctrine/dbal/blob/2.12.x/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php#L353

But perhaps I'm missing something?!

[Adnan-cds]

Hi Finn, James,
I hope to start looking into this soon. Sorry for the constant delay.

I have noticed some interesting precedents in Drupal core where tests have been skipped for SQLite:

• In Drupal 8
• In Drupal 7

If we totally fail to fix this test for SQLite, we can consider taking a similar route.

[Adnan-cds]

Hi Andy, Finn, James,
I have looked into the SQLite issue. No good news I am afraid. Everything Andy and James have said is right. DBAL cannot deal with prefixed database syntax because when its SQLite code was written 9 years ago, SQLite probably didn't work with multiple databases in a single query back then. Hence the code does what it does. But this is an easy fix.

I followed James' suggestion and updated the DBAL and PreviousNext's DBAL wrapper to work with prefixed table names. Quite predictably, I saw the same errors as James:
PDOException: SQLSTATE[HY000]: General error: 1 no such table: test81804115.nested_set_localgov_subsites_parent_node. James has already linked to the entity_hierarchy module code that tries to create this table and this where it could have done better. If you look at the code, you will noticed that if it fails to create the table due to a PHP exception, it ignores the exception and just returns FALSE. I examined the thrown exception and it says this:

An exception occurred while executing 'CREATE TABLE test81804115.nested_set_localgov_subsites_parent_node (id INTEGER UNSIGNED NOT NULL, revision_id INTEGER UNSIGNED NOT NULL, left_pos INTEGER UNSIGNED NOT NULL, right_pos INTEGER UNSIGNED NOT NULL, depth INTEGER UNSIGNED NOT NULL, PRIMARY KEY(id, revision_id))':

SQLSTATE[HY000]: General error: 5 database is locked

Note that this is happening within a test where config is getting imported. The database is probably locked because Drupal's config import process (or some other part) has locked it. Wikipedia puts it nicely:

Several read accesses can be satisfied in parallel. A write access can only be satisfied if no other accesses are currently being serviced.

The D.o ticket that Andy suggests is also to do with locking. It even has a patch.

It may be possible to enable concurrent writes for SQLite. I will see if I can find anything on this. We can also try the patch attached to that.

Finn's latest change passes the tests but fails due to coding standard error. So for now, I am happy to go with that. Perhaps we can open a new ticket to look into running this module's tests under SQLite.

[finnlewis]

Thanks for looking @Adnan-cds

As you mentioned, I've added the check for SQLite connection to each of the failing tests, so it will just skip the tests if running in sqlite. Not ideal, but hopefully OK for now.

The tests do now pass.

I've created an issue to track further attempts as getting the tests working in SQLite #11

[Adnan-cds]

Massive thanks to everyone who has worked on this difficult one :)