Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BC Break] 2.7 has undocumented bc breaks cause of namespace changes (e.g. by removal of 'Doctrine\Bridge') #4694

Closed
robertfausk opened this issue Mar 29, 2022 · 7 comments · Fixed by #4696

Comments

@robertfausk
Copy link

robertfausk commented Mar 29, 2022

API Platform version(s) affected: 2.7/dev-main

Description
As @laryjulien stated in #4556 (comment)

FQCN (fully qualified class name) are not updated in CHANGELOG.md

see e.g.:
10827bb
image

How to reproduce

Implement a class to filter subresources according to solution in #2253 (comment) and update from 2.6.8 to 2.7/dev-main.

This will result in Type error: App\Doctrine\DefaultOrderExtensionDecorator::__construct(): Argument #1 ($decorated) must be of type ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\OrderExtension, ApiPlatform\Doctrine\Orm\Extension\OrderExtension given. See example code below.

Changing use ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\OrderExtension; to use ApiPlatform\Doctrine\Orm\Extension\OrderExtension; fixes the error in the example code.

#services.yml
    App\Doctrine\DefaultOrderExtensionDecorator:
        decorates: api_platform.doctrine.orm.query_extension.order
        arguments:
            $decorated: '@App\Doctrine\DefaultOrderExtensionDecorator.inner'
            $orderParameterName: '%api_platform.collection.order_parameter_name%'
<?php
declare(strict_types=1);

namespace App\Doctrine;

use ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\ContextAwareQueryCollectionExtensionInterface;
use ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\OrderExtension;
use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface;
use App\Entity\Event;
use Doctrine\ORM\QueryBuilder;

final class DefaultOrderExtensionDecorator implements ContextAwareQueryCollectionExtensionInterface
{
    private OrderExtension $decorated;

    private string $orderParameterName;

    public function __construct(OrderExtension $decorated, string $orderParameterName)
    {
        $this->decorated = $decorated;
        $this->orderParameterName = $orderParameterName;
    }

    /**
     * @inheritDoc
     */
    public function applyToCollection(
        QueryBuilder $queryBuilder,
        QueryNameGeneratorInterface $queryNameGenerator,
        string $resourceClass,
        string $operationName = null,
        array $context = []
    ) {
        if ($resourceClass !== Event::class) {
            return;
        }

        // Apply default resource order AFTER none or only some query filters are defined.
        $filters = $context['filters'] ?? [];
        $orderFilters = (array) ($filters[$this->orderParameterName] ?? []);
        $sanitizedOrderFilters = \array_filter($orderFilters);
        $rootAlias = $queryBuilder->getRootAliases()[0];
        if (!isset($sanitizedOrderFilters['date'])) {
            $sanitizedOrderFilters['date'] = 'asc';
            $context['filters'][$this->orderParameterName] = $sanitizedOrderFilters;
            $queryBuilder->addOrderBy(\sprintf('%s.date', $rootAlias), 'asc');
        }
        if (!isset($sanitizedOrderFilters['start'])) {
            $sanitizedOrderFilters['start'] = 'asc';
            $context['filters'][$this->orderParameterName] = $sanitizedOrderFilters;
            $queryBuilder->addOrderBy(\sprintf('%s.start', $rootAlias), 'asc');
        }
        if (!isset($sanitizedOrderFilters['id'])) {
            $sanitizedOrderFilters['id'] = 'asc';
            $context['filters'][$this->orderParameterName] = $sanitizedOrderFilters;
            $queryBuilder->addOrderBy(\sprintf('%s.id', $rootAlias), 'asc');
        }

        $this->decorated->applyToCollection(
            $queryBuilder,
            $queryNameGenerator,
            $resourceClass,
            $operationName,
            $context
        );
    }
}

Possible Solution

  • document all namespace changes as BC breaks for 2.7 in CHANGELOG.md
    OR:
  • prevent BC break by e.g.:
    • provide stub classes which implements/extends the classes in their new location with the old namespace
    • mark these stub classes as deprecated
    • document namespace deprecations in CHANGELOG.md

Additional Context

  • I stumbled across the tweet of @soyuka today and tried dev-main on my api-platform projects.
  • I did not check all namespace changes. Maybe there are more than Core\Bridge

Edit: clarified which exception is occuring.

@soyuka
Copy link
Member

soyuka commented Mar 29, 2022

It should not break we introduced class aliases for every namespace changes in https://github.com/api-platform/core/blob/main/src/deprecated_interfaces.php#L37 (used in https://github.com/api-platform/core/blob/main/src/deprecation.php). I'll try to use the above code and to reproduce but it'd be faster if you had a repository that I can try. Also please tell me how you installed the main branch.
Many thanks for the report!

@robertfausk
Copy link
Author

Here we go:
https://github.com/robertfausk/demo-1

steps to reproduce:

  • checkout
  • docker-compose up -d
  • docker-compose exec php vendor/bin/simple-phpunit --filter EventsTest (Tests are passing)
  • use dev-main in composer.json for api-platform-core
  • docker-compose exec php composer update
  • docker-compose exec php vendor/bin/simple-phpunit --filter EventsTest
    • Tests are failing cause of namespace issue with ApiPlatform\Core\Bridge\Symfony\Bundle\Test\Client
1) App\Tests\Api\EventsTest::testGetAllEventsWithDefaultOrderExtensionDecoratorApplied
TypeError: Cannot assign ApiPlatform\Symfony\Bundle\Test\Client to property App\Tests\Api\EventsTest::$client of type ApiPlatform\Core\Bridge\Symfony\Bundle\Test\Client

/srv/api/tests/Api/EventsTest.php:19
  • rename use ApiPlatform\Core\Bridge\Symfony\Bundle\Test\Client; to use ApiPlatform\Symfony\Bundle\Test\Client; (without Core\Bridge\) manually to get a step further to the issue described above
  • docker-compose exec php vendor/bin/simple-phpunit --filter EventsTest
    • Tests are failing
1) App\Tests\Api\EventsTest::testGetAllEventsWithDefaultOrderExtensionDecoratorApplied
TypeError: App\Doctrine\DefaultOrderExtensionDecorator::__construct(): Argument #1 ($decorated) must be of type ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\OrderExtension, ApiPlatform\Doctrine\Orm\Extension\OrderExtension given, called in /srv/api/var/cache/test/Container6yxY37H/getDefaultOrderExtensionDecoratorService.php on line 24

Btw: My PhpStorm is also not able to link the namespaces after upgrade to dev-main even if it's defined in deprecation.php and working while execution. Have a look at image appended. Should I open a separate issue for this?
image

@soyuka
Copy link
Member

soyuka commented Mar 31, 2022

Nice thanks !

@soyuka
Copy link
Member

soyuka commented Mar 31, 2022

20220331_11h10m00s_grim
gotcha

@soyuka
Copy link
Member

soyuka commented Mar 31, 2022

about phpstorm not sure I'm able to do anything, it should work if it loads the deprecation file, could you give it another try with my last fix?

    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/soyuka/core"
        }
    ]

@soyuka
Copy link
Member

soyuka commented Mar 31, 2022

mhh this is creating another issue I'm still looking :/

@soyuka
Copy link
Member

soyuka commented Apr 5, 2022

fixed and phpstorm shouldn't complain anymore, many thanks for the issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants