Add support for jackalope 2.0 and doctrine phpcr bundle 3.0

[alexander-schranz]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes (Row -> RowInterface)
Deprecations?	no
Fixed tickets	fixes #
Related issues/PRs
License	MIT
Documentation PR	sulu/sulu-docs#

What's in this PR?

Test CI against jackalope 2.0@dev version.

Why?

A new version is in work by @dbu which uses static typings and get ride of doctrine/cache.

Waiting

• upgrade to jackalope 2 jackalope/jackalope-doctrine-dbal#386
• upgrade to jackalope 2 jackalope/jackalope-jackrabbit#166
• use static typing jackalope/jackalope#365
• Add return types for Jackalope 2.0 compatibility doctrine/DoctrinePHPCRBundle#369
• phpcr-shell Test CI against jackalope 2.0@dev version phpcr/phpcr-shell#214
• dantleech/phpcr-migrations?
• dantleech/phpcr-migrations-bundle Test against phpcr bundle 3.0@dev dantleech/phpcr-migrations-bundle#21

[alexander-schranz]

The doctrine/phpcr-bundle still installs doctrine/cache ^1.0. Which should be removed from that bundle as it does not even use that package.

[dbu]

no longer by phpcr-bundle, but by a lot of other parts of doctrine (at least those would allow ^2.0 too):

$ composer why doctrine/cache
sulu/sulu                3.0.x-dev requires doctrine/cache (^1.0.1)            
doctrine/dbal            3.6.6     requires doctrine/cache (^1.11|^2.0)        
doctrine/doctrine-bundle 2.10.2    requires doctrine/cache (^1.11 || ^2.0)     
doctrine/orm             2.15.5    requires doctrine/cache (^1.12.1 || ^2.1.1) 
doctrine/persistence     2.5.7     requires doctrine/cache (^1.11 || ^2.0) 

[alexander-schranz]

Requires: phpcr/phpcr-shell#214

[alexander-schranz]

I rebased this branch to run against ^2.0@dev currentl 2 tests are failing:

There were 2 errors:

1) Sulu\Component\DocumentManager\Tests\Unit\NodeHelperTest::testCopy
Prophecy\Exception\Call\UnexpectedCallException: Unexpected method call on Double\Jackalope\Workspace\P175:
  - copy(
        "/path/to/node",
        "uuid/node"
    )
expected calls were:
  - copy(
        exact("/path/to/node"),
        exact("/path/to/some/other/node/node")
    )

/home/runner/work/sulu/sulu/vendor/phpspec/prophecy/src/Prophecy/Call/CallCenter.php:203
/home/runner/work/sulu/sulu/vendor/phpspec/prophecy/src/Prophecy/Call/CallCenter.php:160
/home/runner/work/sulu/sulu/vendor/phpspec/prophecy/src/Prophecy/Prophecy/ObjectProphecy.php:218
/home/runner/work/sulu/sulu/vendor/phpspec/prophecy/src/Prophecy/Prophet.php:129
/home/runner/work/sulu/sulu/vendor/phpspec/prophecy-phpunit/src/ProphecyTrait.php:59

2) Sulu\Component\DocumentManager\Tests\Unit\NodeHelperTest::testCopyWithDestinationName
Prophecy\Exception\Call\UnexpectedCallException: Unexpected method call on Double\Jackalope\Workspace\P175:
  - copy(
        "/path/to/node",
        "uuid/new-node"
    )
expected calls were:
  - copy(
        exact("/path/to/node"),
        exact("/path/to/some/other/node/new-node")
    )

/home/runner/work/sulu/sulu/vendor/phpspec/prophecy/src/Prophecy/Call/CallCenter.php:203
/home/runner/work/sulu/sulu/vendor/phpspec/prophecy/src/Prophecy/Call/CallCenter.php:160
/home/runner/work/sulu/sulu/vendor/phpspec/prophecy/src/Prophecy/Prophecy/ObjectProphecy.php:218
/home/runner/work/sulu/sulu/vendor/phpspec/prophecy/src/Prophecy/Prophet.php:129
/home/runner/work/sulu/sulu/vendor/phpspec/prophecy-phpunit/src/ProphecyTrait.php:59

Not sure if this is an unexpect regression in PHPCR or an error on our side.

[alexander-schranz]

I did do again a rebase the following errors currently appear:

In DefinitionErrorExceptionPass.php line 49:
                                                                               
  You have requested a non-existent parameter "doctrine_phpcr.odm.document_ma  
  nagers".                                                                                            

Strangely this parameter seems not be used from Sulu but should be defined by the phpcr bundle:

Update: Parameter was defined with a default value previously here: https://github.com/doctrine/DoctrinePHPCRBundle/blob/c59d7449cd2d264af3ff2b510a30853e6f6f78ca/src/Resources/config/phpcr.xml#L9

[alexander-schranz]

Temporary fix for: #6834 (comment) should be fixed in the DoctrinePHPCRBundle

[dbu]

fixing in doctrine/DoctrinePHPCRBundle#386

[alexander-schranz]

Next Error seems related to the new cache configuration.

Not sure if we or phpcrbundle need to require psr/simple-cache /cc @dbu?

Atleast ^3.0 of psr/simple-cache is not yet supported:

[alexander-schranz]

we should also remove this requirement from Sulu this would mostly mean small BC Breaks or create a copy of ArrayCache of Doctrine.

[dbu]

i can reproduce the failing unit tests with NodeHelperTest

looking at the NodeHelper, i can't imagine why this has worked in the past. you use 'uuid' as node id, instead of a valid uuid string. the NodeHelper then in normalizePath checks if this is a uuid and if not, returns the literal path that was passed.

i am also not quite sure why there is a call to a prophesized Workspace object to the copy method. i am not familiar with prophecy but it seems to me like you try to get it to actually copy the node, but without the object manager that will not do anything. apart from the : void return type, the Workspace::copy method has not changed.

[alexander-schranz]

@dbu I rebased the branch to test against current version. There appears a new error which I maybe think we need to change something:

TypeError: Doctrine\Bundle\PHPCRBundle\ManagerRegistry::__construct(): Argument #5 ($defaultEntityManagerName) must be of type string, null given, called in /home/runner/work/sulu/sulu/src/Sulu/Bundle/SearchBundle/Tests/Application/var/cache/admin/test/ContainerOEjlAQ2/getDoctrinePhpcrService.php on line 21 and defined in /home/runner/work/sulu/sulu/vendor/doctrine/phpcr-bundle/src/ManagerRegistry.php:20

Another issue appears on prefer lowest but maybe it just add a conflict to 2.0.0-beta1 🤔

Updated Solved by: cde0dae

37. Sulu\Bundle\PageBundle\Tests\Functional\Mapper\ContentMapperTest::testDelete
    TypeError: Jackalope\ObjectManager::pathArrayToPropertiesIterator(): Argument Create a scripthandler #1 ($propertyPaths) must be of type string, array given, called in /home/runner/work/sulu/sulu/vendor/jackalope/jackalope/src/Jackalope/ObjectManager.php on line 689

[mamazu]

Well the last test that's wrong that's broken, doesn't sound like a problem with this PR directly. I have noticed that the "lowest version" of the dbal package is now updated.

[alexander-schranz]

I did change the composer requirements so both versions are now supported tests currently failing because it does install phpcr bundle 2.5 which is not compatible with jackalope 2 but phpcr bundle does not define its requirements for jackaloe.

The installation of phpcr bundle 3.0 is blocked by phpcr/phpcr-migrations-bundle#4

[dbu]

it does install phpcr bundle 2.5 which is not compatible with jackalope 2 but phpcr bundle does not define its requirements for jackalope.

i could add a conflict to latest phpcr-bundle, but i guess that does not help much as composer would just pick an older version to satisfy the solution.
otherwise i could try to make phpcr-bundle 2.x compatible with jackalope 2, but probably a bit wasted effort, lets rather get phpcr-bundle 3 working and use that.

[alexander-schranz]

i could add a conflict to latest phpcr-bundle, but i guess that does not help much as composer would just pick an older version to satisfy the solution.

We would need to add conflict to ^2.5 version and sulu then would require to require atleast that version that way it would work, as composer then can not just downgrade phpcr bundle.

[dbu]

We would need to add conflict to ^2.5 version and sulu then would require to require atleast that version that way it would work, as composer then can not just downgrade phpcr bundle.

you mean a new patch version of the phpcr-bundle that declares a conflict with jackalope 2? or a conflict in sulu (that could be conflicts: {"doctrine/phpcr-bundle": "<3.0"} if you don't require phpcr-bundle directly)

[dbu]

yay, that looks pretty green now \0/

looks like whats left now is mainly about the added strict typing in upstream libraries that makes phpstan report missing checks for null and similar things.

[alexander-schranz]

Ready for review. @chirimoya