Adds PHP 8 support and Laminas CI workflow #6

Slamdunk · 2021-04-28T09:05:22Z

No description provided.

Signed-off-by: Filippo Tessarotto <[email protected]>

Slamdunk · 2021-05-03T12:16:57Z

Rebase done, now an approval is needed

Slamdunk · 2021-05-03T12:35:21Z

The build fails on locked runs for PHP prior to 7.4.
I've indeed committed composer.lock after updating it on PHP 7.4.

What should I do @boesing ? Generate a lock for by PHP 5.6? I'd prefer remove it altogether, I don't see how it can help on such a wide range of PHP versions that, ndr, compile good on both lowest and latest

boesing · 2021-05-03T20:31:55Z

@Slamdunk drop support for PHP <7.3
I don't think that I'll create a new release just with the GHA added, so we can wait until we implement #3

Thats what I did in almost all other cache adapters.

boesing · 2021-05-03T20:55:19Z

Oh by the way, please remove the markTestSkipped from the test cases so we can verify that we are actually testing anything. Due to the CI output, all tests are actually skipped.

You may have a look at how I've done that for the redis adapter:
https://github.com/laminas/laminas-cache-storage-adapter-redis/blob/237fbc02ee4c5eeb6ef8fe7e3bf6ec55ef99f422/.github/workflows/continuous-integration.yml#L26
https://github.com/laminas/laminas-cache-storage-adapter-redis/blob/237fbc02ee4c5eeb6ef8fe7e3bf6ec55ef99f422/.github/workflows/continuous-integration.yml#L41

Maybe #1 can help to make some tests pass as I've experienced issues in v1.0.0 already but that version was meant to be a 1:1 copy of the original laminas-cache code.

Signed-off-by: Filippo Tessarotto <[email protected]>

Slamdunk

@boesing So, this is a tough one. I had no prior knowledge of MongoDB before this PR, please be forgiving.

I've managed to get 414 out of 416 tests pass, with only 2 failures, regarding binary data:

There was 1 error:

1) LaminasTest\Cache\Psr\SimpleCache\ExtMongoDbIntegrationTest::testBinaryData
Laminas\Cache\Psr\SimpleCache\SimpleCacheException: Detected invalid UTF-8 for field path "value": s:256:"

/vendor/laminas/laminas-cache/src/Psr/SimpleCache/SimpleCacheDecorator.php:307
/vendor/laminas/laminas-cache/src/Psr/SimpleCache/SimpleCacheDecorator.php:125
/vendor/cache/integration-tests/src/SimpleCacheTest.php:694

Caused by
Laminas\Cache\Exception\RuntimeException: Detected invalid UTF-8 for field path "value": s:256:"

/src/ExtMongoDb.php:214
/vendor/laminas/laminas-cache/src/Storage/Adapter/AbstractAdapter.php:673
/vendor/laminas/laminas-cache/src/Psr/SimpleCache/SimpleCacheDecorator.php:123
/vendor/cache/integration-tests/src/SimpleCacheTest.php:694

Caused by
MongoDB\Driver\Exception\UnexpectedValueException: Detected invalid UTF-8 for field path "value": s:256:"

/vendor/mongodb/mongodb/src/Operation/InsertOne.php:132
/vendor/mongodb/mongodb/src/Collection.php:931
/src/ExtMongoDb.php:212
/vendor/laminas/laminas-cache/src/Storage/Adapter/AbstractAdapter.php:673
/vendor/laminas/laminas-cache/src/Psr/SimpleCache/SimpleCacheDecorator.php:123
/vendor/cache/integration-tests/src/SimpleCacheTest.php:694

--

There was 1 failure:

1) LaminasTest\Cache\Psr\CacheItemPool\ExtMongoDbIntegrationTest::testBinaryData
Binary data must survive a round trip.
Failed asserting that false is true.

/vendor/cache/integration-tests/src/CachePoolTest.php:763

I'm not able to go further 😞

Slamdunk · 2021-05-04T08:46:51Z

.github/workflows/continuous-integration.yml

+    runs-on: ${{ matrix.operatingSystem }}
+    services:
+      mongo:
+        image: mongo


I have no idea what health check to use for MongoDB.

Slamdunk · 2021-05-04T08:47:44Z

composer.json

@@ -16,13 +16,15 @@
    "laminas/laminas-cache-storage-implementation": "1.0"
  },
  "require-dev": {
-    "laminas/laminas-cache": "^2.10",
-    "laminas/laminas-cache-storage-adapter-test": "^1.0@dev",
+    "mongodb/mongodb": "^1.8.0",


I was surprised this requirement wasn't there before: without it no test passes

It was required by the travis configuration as there were different versions used for different php versions.

Slamdunk · 2021-05-04T08:50:05Z

src/ExtMongoDb.php

@@ -134,6 +134,8 @@ protected function internalGetItem(& $normalizedKey, & $success = null, & $casTo
            return;
        }

+        self::ensureArrayType($result);


This is needed not only for the following array_key_exists call, but also for array subtypes which, without it, are kept \MongoDB\Model\BSONDocument

Slamdunk · 2021-05-04T08:51:30Z

src/ExtMongoDbResourceManager.php

@@ -51,8 +51,7 @@ public function setResource($id, $resource)
    {
        if ($resource instanceof Collection) {
            $this->resources[$id] = [
-                'db'                  => (string) $resource->db,
-                'db_instance'         => $resource->db,


I have no idea what db was in the past, but now it's not alive anymore 🤷

Slamdunk · 2021-05-04T13:15:45Z

I saw testBinaryData is skipped in other adapters, we can then skip it here too if you agree

boesing · 2021-05-04T16:08:06Z

I've never used mongo aswell ;-)

I've created #1 months ago to fix that BSON problem but as it is not fixed at the moment, I would dev-require that version which was used in travis as the highest version.

After this, we create a feature PR where we are supporting old and newer versions of the extension.
So to achieve that the proper extension version is installed, we have to use PECL.
I've added a PR to the CI Action a few weeks back to support pecl on CLI.
So what we have to do is:

remove mongodb extension from the .laminas-ci.json
create a mongodb-extension-installer.sh under .laminas-ci/
add that file to the pre-install.sh
Implement switch/case to that file and use pecl to install specific versions of the mongodb extension based on the PHP version (as done in the travis config before)

Doing this, we can ensure that we create the same "deps" as done in the travis build.
This way, we can add support for newer pecl extension versions easier.
Maybe, we use latest pecl version for e.g. the latest PHP version we support (e.g. PHP 8), but thats just an idea.

Slamdunk · 2021-05-05T07:23:11Z

Maybe #1 can help to make some tests pass as I've experienced issues in v1.0.0 already but that version was meant to be a 1:1 copy of the original laminas-cache code.

Gosh I completely missed this reference 😭

I've created #1 months ago to fix that BSON problem but as it is not fixed at the moment

What do you mean "it is not fixed at the moment"? This is the purpose of #1 a this PR, isn't it?

I would dev-require that version which was used in travis as the highest version.

I'm sorry I don't get this: "I would dev-require that version" of what?

After this, we create a feature PR where we are supporting old and newer versions of the extension.

mongodb/mongodb library already restricts ext-mongodb support:
https://github.com/mongodb/mongo-php-library/blob/df4e32cf506095ef03824638be88b6e06d6e8cc5/composer.json#L15

I think this is enough to let us ignore multiple ext-mongodb versions and only support latest one, isn't it?

I've added a PR to the CI Action a few weeks back to support pecl on CLI.

I miss the PR you are referring to :\

boesing · 2021-05-05T12:13:12Z

What do you mean "it is not fixed at the moment"? This is the purpose of #1 a this PR, isn't it?

Yes but that PR is kinda outdated. And this is why the bug is still present when mongodb/mongodb is used in v1.6.0+.
AFAIR, the mongodb/mongbd returned other types due to BC breaks and thus this component is not compatible with 1.6.0+ of mongodb/mongodb. I dont think we have to fix that in this PR.

I'm sorry I don't get this: "I would dev-require that version" of what?

mongodb/mongodb, for this PR, you might want to require that dependency <1.6.0 probably.

I miss the PR you are referring to :\

laminas/laminas-continuous-integration-action#20

Slamdunk · 2021-05-05T13:46:46Z

I'm sorry I'm more confused than ever.

I dont think we have to fix that in this PR.

Why not? I mean, I understand we are looking for atomic PR, but as we decided to drop PHP <7.3, we might as well make it compatible with PHP >= 7.3

mongodb/mongodb, for this PR, you might want to require that dependency <1.6.0 probably.

Buy why? Why not simply have "mongodb/mongodb": "^1.8.0" in the require section?
Why should we test and support older versions when the latest one already passes the CI in all the PHP versions?

boesing · 2021-05-05T17:37:58Z

Why not? I mean, I understand we are looking for atomic PR, but as we decided to drop PHP <7.3, we might as well make it compatible with PHP >= 7.3

https://github.com/mongodb/mongo-php-library/releases/tag/1.5.0

That version is absolutely compatible with PHP 7.3 & PHP 7.4.

Buy why? Why not simply have "mongodb/mongodb": "^1.8.0" in the require section?

I think that would be indeed possible.

But a concern I have: there might be projects which update this package and end up having another unused dependency just because we still require this package in laminas-cache. So thats why I struggle adding new dependencies to the 1.x version of the satellites.

This is why I've added conflict to the #1 PR rather than require that dependency, but I am actually not that familiar with all these dependency stuff in OSS.

Maybe @Ocramius has a view on this. I'd prefer having no new hard requirements for the satellites (in v1.x) to avoid potential fuckups in upstream projects which do not even use this component.

Slamdunk · 2021-05-06T07:04:39Z

But a concern I have: there might be projects which update this package and end up having another unused dependency just because we still require this package in laminas-cache. So thats why I struggle adding new dependencies to the 1.x version of the satellites.

This is why I've added conflict to the #1 PR rather than require that dependency, but I am actually not that familiar with all these dependency stuff in OSS.

Seems a correct concern and a good solution to me: what about a "conflict": {"mongodb/mongodb": "<1.8"} then?

boesing · 2021-05-06T07:16:21Z

what about a "conflict": {"mongodb/mongodb": "<1.8"} then?

That could be a way, but what if someone depends on that library in another use-case?
Well, we could ofc provide some guidance on how to remove this adapter from the project by using the guide:
https://github.com/laminas/laminas-cache/tree/2.11.x#avoid-unused-cache-adapters-are-being-installed

Sounds good enough to me. Go for it 👍🏼

Signed-off-by: Filippo Tessarotto <[email protected]>

boesing · 2021-05-06T07:53:56Z

Thanks @Slamdunk!
The only missing PR is PHP 8.0 support and then we're good to go 🎉
Great work here!

Slamdunk · 2021-05-06T07:55:14Z

The only missing PR is PHP 8.0 support

It's already here https://github.com/laminas/laminas-cache-storage-adapter-ext-mongodb/pull/6/files#diff-d2ab9925cad7eac58e0ff4cc0d251a937ecf49e4b6bf57f8b95aab76648a9d34R10

boesing · 2021-05-06T11:56:12Z

It's already here

🤦🏼‍♂️ I definitely have to be more focused these days.

This comment has been minimized.

Sign in to view

boesing added the Enhancement label Apr 29, 2021

boesing added this to the 1.1.0 milestone Apr 29, 2021

Adopt Laminas CI workflow

866656d

Signed-off-by: Filippo Tessarotto <[email protected]>

Slamdunk force-pushed the laminas_ci_workflow branch from 3e78f4e to 866656d Compare May 3, 2021 12:16

Slamdunk added 5 commits May 4, 2021 08:32

Require PHP 7.3, support PHP 8.0

0d3df63

Signed-off-by: Filippo Tessarotto <[email protected]>

Require mongodb/mongodb extension to work

682e041

Signed-off-by: Filippo Tessarotto <[email protected]>

Update phpunit.xml.dist to PHPUnit 9

8aba30e

Signed-off-by: Filippo Tessarotto <[email protected]>

Github Actions: use mongo image

fd84804

Signed-off-by: Filippo Tessarotto <[email protected]>

Fixes to get the tests pass; only 3 failures remaining

4a5aa31

Signed-off-by: Filippo Tessarotto <[email protected]>

Slamdunk marked this pull request as draft May 4, 2021 08:01

Fix mongodb service hostname

a0d9ff4

Signed-off-by: Filippo Tessarotto <[email protected]>

Slamdunk marked this pull request as ready for review May 4, 2021 08:15

Ensure results are array

ea47ba2

Signed-off-by: Filippo Tessarotto <[email protected]>

Slamdunk commented May 4, 2021

View reviewed changes

Slamdunk mentioned this pull request May 4, 2021

PHP 8.0 support for v2 laminas/laminas-cache#88

Closed

10 tasks

Slamdunk added 2 commits May 6, 2021 09:27

Skip Binary data tests

56e260e

Signed-off-by: Filippo Tessarotto <[email protected]>

Import #1 diff

e93011b

Signed-off-by: Filippo Tessarotto <[email protected]>

Conflict with mongodb < 1.8

c77a16e

Signed-off-by: Filippo Tessarotto <[email protected]>

boesing mentioned this pull request May 6, 2021

remove old php versions #1

Closed

boesing merged commit 5810c3f into laminas:1.1.x May 6, 2021

Slamdunk deleted the laminas_ci_workflow branch May 6, 2021 07:53

froschdesign linked an issue May 6, 2021 that may be closed by this pull request

PHP 8.0 support #3

Closed

6 tasks

froschdesign changed the title ~~Adopt Laminas CI workflow~~ Adds PHP 8 support and Laminas CI workflow May 6, 2021

boesing mentioned this pull request May 6, 2021

PHP 8.0 support #3

Closed

6 tasks

Uh oh!

Adds PHP 8 support and Laminas CI workflow #6

Adds PHP 8 support and Laminas CI workflow #6

Uh oh!

Conversation

Slamdunk commented Apr 28, 2021

Uh oh!

This comment has been minimized.

Slamdunk commented May 3, 2021

Uh oh!

Slamdunk commented May 3, 2021

Uh oh!

boesing commented May 3, 2021

Uh oh!

boesing commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Slamdunk left a comment

Choose a reason for hiding this comment

Uh oh!

Slamdunk May 4, 2021

Choose a reason for hiding this comment

Uh oh!

Slamdunk May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boesing May 4, 2021

Choose a reason for hiding this comment

Uh oh!

Slamdunk May 4, 2021

Choose a reason for hiding this comment

Uh oh!

Slamdunk May 4, 2021

Choose a reason for hiding this comment

Uh oh!

Slamdunk commented May 4, 2021

Uh oh!

boesing commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Slamdunk commented May 5, 2021

Uh oh!

boesing commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Slamdunk commented May 5, 2021

Uh oh!

boesing commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Slamdunk commented May 6, 2021

Uh oh!

boesing commented May 6, 2021

Uh oh!

boesing commented May 6, 2021

Uh oh!

Slamdunk commented May 6, 2021

Uh oh!

boesing commented May 6, 2021

Uh oh!

Uh oh!

boesing commented May 3, 2021 •

edited

Loading

Slamdunk May 4, 2021 •

edited

Loading

boesing commented May 4, 2021 •

edited

Loading

boesing commented May 5, 2021 •

edited

Loading

boesing commented May 5, 2021 •

edited

Loading