Support PHP8 attributes, and prefer them over annotations if present

[Richard87]

I added #[Attribute] to the Breadcrumb annotation, and updated the BreadcrumListener to look for them, if supported by your PHP Version, and if the new type configuration is set to attribute, or both (new default).

Note, annotations are parsed first, so if you mix them on a method you probably want to use the position property.

New configuration:

apy_breadcrumb_trail:
  type: both # could be one of "attribute", "annotation" or "both"
  template: APYBreadcrumbTrailBundle::breadcrumbtrail.html.twig

Some of the ways to specify a breadcrum:

/** @Breadcrumb("{course}", route={"name"="courses_detail", "parameters"={"shortTitle"="{course.shortTitle}"}}, routeAbsolute=true) */
#[Breadcrumb(title: '{course}', route: ['name' => 'courses_detail', 'parameters' => ['shortTitle' => '{course.shortTitle}']], routeAbsolute: true)]
#[Breadcrumb(title: '{course}', routeName: 'courses_detail', routeParameters: ['shortTitle' => '{course.shortTitle}'], routeAbsolute: true)]

Rector script to automatic upgrade:

return static function (ContainerConfigurator $containerConfigurator): void {
    // get parameters
    $parameters = $containerConfigurator->parameters();
    $parameters->set(Option::PATHS, [__DIR__ . '/src']);
    $parameters->set(Option::SYMFONY_CONTAINER_XML_PATH_PARAMETER, __DIR__ . '/var/cache/dev/App_KernelDevDebugContainer.xml');
    $parameters->set(Option::AUTO_IMPORT_NAMES, true);
    $parameters->set(Option::IMPORT_SHORT_CLASSES, false);

    $services = $containerConfigurator->services();
    $services->set(AnnotationToAttributeRector::class)->call('configure', [
        [
            AnnotationToAttributeRector::ANNOTATION_TO_ATTRIBUTE => ValueObjectInliner::inline([
                new AnnotationToAttribute(\APY\BreadcrumbTrailBundle\Annotation\Breadcrumb::class),
            ]),
        ],
    ]);
};

[rvanlaak]

Looks great! The type parameter is not needed, as the Attributes can get implemented backwards compatible. WDYT?

[Richard87]

Thanks, I did it specifically so I could disable annotations and skip that overhead :)

By default everything is backwards compatible right now, with the default both

Specifically, the getAttributes method returns an empty array if not supported:

    /**
     * @param \ReflectionClass|\ReflectionMethod $reflected
     * @return list<Breadcrumb>
     */
    private function getAttributes($reflected): array
    {

        if (\PHP_VERSION_ID < 80000) {
            return [];
        }

        $attributes = [];
        foreach ($reflected->getAttributes(Breadcrumb::class) as $reflectionAttribute) {
            $attributes[] = $reflectionAttribute->newInstance();
        }

        return $attributes;
    }

[Richard87]

Hi! I changed the "data" argument to "title" for "more correctness" :)

So we are using it like this in production now:

#[Route(path: '/dashboard/SKIL')]
#[Security(data: "is_granted('ROLE_SKIL')")]
#[Breadcrumb(title: 'SKIL', route: ['name' => 'skil_dashboard'], routeAbsolute: true)]
class SkilController extends Controller {

    #[Route(path: '/recordings/{id}', name: 'skil_recordings_get')]
    #[Breadcrumb('Opptak', routeName: 'skil_recordings', routeAbsolute: true)]
    #[Breadcrumb('{id}')]
    public function getRecordingAction(string $id, RecordingRepository $recordingRepo): Response {
        /** @var Recording $recording */
        $recording = $recordingRepo->find($id);
        return $this->render('Api/recording.html.twig', [
            'recording' => $recording,
        ]);
    }

[Richard87]

Hi @rvanlaak , do you still think that I should removed the extra configuration "type", or are you okay with keeping it? :)

also, would love to rebase this PR ontop of the other "fix test" PR I submittet to get the tests running again ;)

[rvanlaak]

The 1.x branch of this bundle is stable for years already, and no new features were added.

Therefor it would be good for this bundle to:

• prefer PHP Attributes
• deprecate all annotations functionality
• marking them to be removed in v3
• and bumping requirements for PHP to 8.0 and Symfony to 4.4.

Would that be something you could help with?

[rvanlaak]

blocked by #65

[Richard87]

Definitly, it would be as easy to say "both" and "annotation" type is deprecated, and that type: attribute is the way to go forward, then in v3 we remove the type parameter :)

We could put it in the Annotation loader part, to trigger a deprecation for eact annotation (would help find annotations), or in the config-level (only 1 deprectation instead of many).

Or we could do both?

I would recommend using Symfony's deprecation contracts package, and using it as this (wherever required):

trigger_deprecation('symfony/package-name', '5.1', 'The "%s" class is deprecated, use "%s" instead.', Deprecated::class, Replacement::class);

(trigger_deprecation is a global method provided by symfony) https://github.com/symfony/deprecation-contracts https://symfony.com/doc/current/contributing/code/conventions.html#deprecating-code

[rvanlaak]

@Richard87 after rebasing your branch on master to get CI green again, and adding a test on the breadcrumb Annotation implementation, I think that we should revise some parts of the implementation.

As you can see on the failing tests, the PHP8.x implementation now finds 6 breadcrumbs where 3 are expected. This will lead to confusion for people that are upgrading their PHP versions. As PHP attributes are BC, we can remove all configuration.

My suggested changes / TODOs:

1. [x] Prefer attributes over annotations. We can do this by throwing an exception in case they were found both to promote upgrading to attributes (you could add the Rector config to a bin/rector-upgrade-annotations-to-attributes script)
2. [x] Remove the configuration. Attributes are BC, and the exception will make clear that only one variation is possible, and attributes are preferred.
3. [x] Add a test on the dynamically parsed breadcrumb parameters, that get derived from a method's parameters (see snippet below). Did you see them work on attributes? I think we are actually relying on the object traversal logics within Twig.
4. [ ] Trigger deprecations on the annotations. Regarding the place where to trigger the deprecation; we want to help people solve their deprecations while running their integration test suite. In other words, let's trigger the deprecation in the BreadcrumbListener and provide details on the place where the annotation was found.

/**
 * @Breadcrumb("{title}")
 **/
public function blogAction(string $title)

[Richard87]

I added a PHP8 only tests, and changed your test to assertGreaterThanOrEqual 3 (we will get skipped tests on old versions, and good tests on the new version)

[rvanlaak]

Am I correct in assuming that the test case you added is mainly to get CI green? Imho we should always expect 3 breadcrumbs in the case of the fixture. It would be weird for users to all of a sudden get six breadcrumbs when they update their version of PHP.

Now I think about it again, that fixture should actually throw an exception, as it mixes attributes and annotations. Mixing them will lead to lots of confusion with regards to the ordering of the annotations. I do not see a clear way how we can write proper documentation for that.

Are you ok if I make the suggested changes to your PR, so we can get this merged and released?

[Richard87]

Yes you are right :)

I assumed some people would want to mix annotations with attributes, but I converted everything at once so I have no strong feelings toward it :)

But, If I understand you correctly, we should load both, but throw deprecations when we encounter a annotation. Then in the next major version, we remove loading annotations?

(if so, I completley agree!)

[Richard87]

I suggest we throw an exception if we mix annotation with attributes on the same "level":

This will throw:

/**
 * @Breadcrumb("first-breadcrumb")
 * @Breadcrumb("second-breadcrumb")
 */
#[Breadcrumb(title: 'first-breadcrumb')]
#[Breadcrumb(title: 'second-breadcrumb')]
class ControllerWithMultipleAttributes extends AbstractController
{
    /**
     * @Breadcrumb("third-breadcrumb")
     */
    #[Breadcrumb(title: 'third-breadcrumb')]
    public function indexAction()
    {
        return [];
    }
}

While this is allowed:

#[Breadcrumb(title: 'first-breadcrumb')]
#[Breadcrumb(title: 'second-breadcrumb')]
class ControllerWithMultipleAttributes extends AbstractController
{
    /**
     * @Breadcrumb("third-breadcrumb")
     */
    public function indexAction()
    {
        return [];
    }
}

That is not "ambigious", and its easy to handle :) (still logging deprecations on all annotations!)

[rvanlaak]

@Richard87 I've created a PR on your repo with a suggestion for the enhanced implementation. Can you take a look at it and merge it if you're okay with that? https://github.com/Richard87/APYBreadcrumbTrailBundle/pull/2

[Richard87]

Merged, I added some tests at the same time, with a Custom Exception with information about which controller::action that has bugs...

The only thing missing is deprecation notice i think? :)

[rvanlaak]

I think your merge on EventListener/BreadcrumbListener went wrong, there is some additional conditional logic in there to not break PHP7.3

We will add the deprecation in another PR.

[Richard87]

I think your merge on EventListener/BreadcrumbListener went wrong, there is some additional conditional logic in there to not break PHP7.3

We will add the deprecation in another PR.

Hmm, I cant find the error? PhpUnit claims a file is there, but has the wrong class. But I can't find the file at all?

[Richard87]

Ill look into the rest later, I got to run! Thanks!

[rvanlaak]

The failing test on "test lowest" CI jobs was related bug https://github.com/doctrine/annotations/pull/276 that was fixed in their 1.7 release. Adding a conflict in our composer.json made CI green again.

[rvanlaak]

This PR is ready to get merged. Created the 1.7 milestone and issue #67 for trigger_deprecations.

Thank you for your efforts @Richard87

[Richard87]

This PR is ready to get merged. Created the 1.7 milestone and issue #67 for trigger_deprecations.

Thank you for your efforts @Richard87

My pleasure, thanks for the help in merging the code!

[rvanlaak]

Only thing I forgot was to squash all commits 🤷

[Richard87]

hehe, yeah, I tried, but wasn't allowed for some reason :O