EZP-30681: Fixed double slash in path_identification_string after moveSubtree

[SerheyDolgushev]

Question	Answer
JIRA issue	EZP-30681
Bug/Improvement	yes
New feature	no
Target version	master
BC breaks	no
Tests pass	yes
Doc needed	no

Use case:

1. Under "Home" location (Add missing read only properties to role structs #2) create two folders: "Parent" and "Move me".
2. Move "Move me" folder to "Parent".
3. Check ezcontentobject_tree.path_identification_string for moved node.

Expected result: node_2/parent/move_me
Actual result: node_2/parent//move_me

TODO:

• Implement feature / fix a bug.
• Implement tests.
• Fix new code according to Coding Standards ($ composer fix-cs).
• Ask for Code Review.

[SerheyDolgushev]

Please note, existing install might require to run SQL to replace // => / in ezcontentobject_tree.path_identification_string

[alongosz]

Unfortunately tests are failing there, so while this fixes some issue in some specific place/for some specific use case, it breaks other parts of the system.

If clean data is incorrect, it should also be a part of the fix.

We cannot accept it in the current form.

[SerheyDolgushev]

@alongosz, please have a look

[alongosz]

The path_identification_string is deprecated and will be dropped in the future, so I would recommend to stop relying on it in favor of Location::$pathString property.

Issues to resolve:

1. This bug exists in 6.7 so the fix need to be targeted to that branch.
2. The issue for me still exists despite the fix.
3. Test case is still missing. Property is not available on API level, so the test is a bit ugly.
   Please add it to eZ/Publish/API/Repository/Tests/LocationServiceTest.php. Please Note that the test case was done on 7.5 branch, so doing it on 6.7 might require some changes.

    /**
     * Test that Legacy ezcontentobject_tree.path_identification_string field is correctly updated
     * after moving subtree.
     *
     * @covers \eZ\Publish\API\Repository\LocationService::moveSubtree
     *
     * @throws \ErrorException
     * @throws \eZ\Publish\API\Repository\Exceptions\ForbiddenException
     * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException
     * @throws \eZ\Publish\API\Repository\Exceptions\UnauthorizedException
     */
    public function testMoveSubtreeUpdatesPathIdentificationString()
    {
        $repository = $this->getRepository();
        $locationService = $repository->getLocationService();

        $topNode = $this->createFolder(['eng-US' => 'top_node'], 2);

        $newParentLocation = $locationService->loadLocation(
            $this
                ->createFolder(['eng-US' => 'Parent'], $topNode->contentInfo->mainLocationId)
                ->contentInfo
                ->mainLocationId
        );
        $location = $locationService->loadLocation(
            $this
                ->createFolder(['eng-US' => 'Move Me'], $topNode->contentInfo->mainLocationId)
                ->contentInfo
                ->mainLocationId
        );

        $locationService->moveSubtree($location, $newParentLocation);

        // path location string is not present on API level, so we need to query database
        $connection = $this->getRawDatabaseConnection();
        $query = $connection->createQueryBuilder();
        $query
            ->select('path_identification_string')
            ->from('ezcontentobject_tree')
            ->where('node_id = :nodeId')
            ->setParameter('nodeId', $location->id);

        self::assertEquals(
            'top_node/parent/move_me',
            $query->execute()->fetchColumn()
        );
    }

[SerheyDolgushev]

@alongosz

1. Rebase is done.
2. The issue is fixed. Please note testMoveHiddenSourceChildUpdate had en edge case: moving location with root path_identification_string. So current fix handles cases like that one.
3. The test is added.

Please review.

[alongosz]

@SerheyDolgushev in the future please fetch latest upstream changes before doing rebase. You've rebased against 6.7 from September 2018, which gave our QA a headache 😜

This time I fixed that for you and force-pushed :) // cc @barbaragr

[lserwatka]

You can merge it up

[alongosz]

Done

[SerheyDolgushev]

@alongosz should a new item be added to PR todo list? Something like "Check that you are using the latest source branch"?

[alongosz]

@alongosz should a new item be added to PR todo list? Something like "Check that you are using the latest source branch"?

Maybe, but I wonder if this would be understood out of context. Moreover some people (not gonna point fingers ;p) tend to check boxes regardless of if they did the thing, so I started to find this pointless :P
At some point I planned to revise PR template anyway.