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

[RFC] Consistently use the word "attribute" in all symbols #51381

Closed
7 tasks done
derrabus opened this issue Aug 14, 2023 · 14 comments · Fixed by #52294
Closed
7 tasks done

[RFC] Consistently use the word "attribute" in all symbols #51381

derrabus opened this issue Aug 14, 2023 · 14 comments · Fixed by #52294
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@derrabus
Copy link
Member

derrabus commented Aug 14, 2023

Description

Doctrine Annotations have served us well, but PHP attributes are the future.

Our strategy for Symfony 6 was to support Doctrine Annotations and PHP attributes as two ways of annotating pieces of configuration to code. In Symfony 7, attributes will be the only supported way. However, we still use the word "annotation" in the codebase in an abstract sense (PHP attributes are a form of annotation). This might be confusing, so we might consider using the work "attribute" everywhere consistently.

I've opened this issue to collect all occurrences of the term and to discuss the possible impact on downstream projects.

  • The serializer integration of FrameworkBundle exposes a configuration option enable_annotations. Rename to enable_attributes.
  • The validation integration of FrameworkBundle exposes a configuration option enable_annotations. Rename to enable_attributes.
  • The Routing and Serializer components organize their attribute classes in a namespace called Annotation. Rename that namespace to Attribute.
  • When loading routes from attributes, the loader type is annotation although attribute is already an alias for that. Make attribute the only possible type.
  • Serializer uses a class named AnnotationLoader to load metadata from attributes. Rename to AttributeLoader.
  • Validation uses a class named AnnotationLoader to load metadata from attributes. Rename to AttributeLoader.
  • Routing contains classes AnnotationClassLoader and AnnotationDirectoryLoader for loading routes from attributes. Rename to AttributeClassLoader and AttributeDirectoryLoader.

If you find more TODOs, please drop a comment.

@carsonbot carsonbot added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Aug 14, 2023
@nicolas-grekas
Copy link
Member

I'm fine with all items except the second one:

The Routing and Serializer components organize their attribute classes in a namespace called Annotation. Rename that namespace to Attribute?

I think this can lead to costly migrations from 6 to 7. The benefit might not be worth the troubles.
If we really want to go this way, we could add the Attribute namespace and keep the Annotation one as an alias. But I wouldn't deprecate in the same version that introduces the alternative.

For validation, I'm also wondering if the move shouldn't do one more step and clean the way we create those objects. Right now, constructors are a bit strange. Maybe relying only on named args is possible?

@derrabus
Copy link
Member Author

The Routing and Serializer components organize their attribute classes in a namespace called Annotation. Rename that namespace to Attribute?

I think this can lead to costly migrations from 6 to 7.

I don't know. In all of the codebases I've seen those attributes in action, they're imported like this:

use Symfony\Component\Routing\Annotation\Route;

This looks like a big search&replace to me. Your favorite IDE should be able to handle that.

@derrabus
Copy link
Member Author

For validation, I'm also wondering if the move shouldn't do one more step and clean the way we create those objects.

I've created #51393 for that. This might be a bit heavier to migrate, I'm afraid.

@stof
Copy link
Member

stof commented Aug 18, 2023

@derrabus a search-and-replace only works if you don't need to support multiple versions of Symfony in your code base.

@nicolas-grekas
Copy link
Member

We should also consider if deprecating and providing the alternative in the same version isn't too harsh. We know this breaks migration paths for third party bundles. We could instead provide the alternative in 6.4, and deprecate in 7.1. We need to decide if this concern apply to the listed cases. WDYT?

@wouterj
Copy link
Member

wouterj commented Aug 18, 2023

I'm not sure if there are many third party bundles that use attributes, given it needs to be enabled by the user for both validation and routing?

derrabus added a commit that referenced this issue Aug 22, 2023
…rrences (alexandre-daubois)

This PR was merged into the 6.4 branch.

Discussion
----------

[FrameworkBundle][Validator] Deprecate annotation occurrences

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | Part of #51381
| License       | MIT
| Doc PR        | -

 * Deprecate `framework.validation.enable_annotations` in favor of `framework.validation.enable_attributes`
 * Deprecate `framework.serializer.enable_annotations` in favor of use `framework.serializer.enable_attributes`
 * Deprecate `ValidatorBuilder::enableAnnotationMapping()` in favor of `ValidatorBuilder::enableAttributeMapping()`
 * Deprecate `ValidatorBuilder::disableAnnotationMapping()` in favor of `ValidatorBuilder::disableAttributeMapping()`
 * Deprecate `AnnotationLoader` in favor of `AttributeLoader`

Commits
-------

2e1e805 [FrameworkBundle][Validator] Deprecate annotation occurrences
symfony-splitter pushed a commit to symfony/http-kernel that referenced this issue Aug 22, 2023
…rrences (alexandre-daubois)

This PR was merged into the 6.4 branch.

Discussion
----------

[FrameworkBundle][Validator] Deprecate annotation occurrences

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | Part of symfony/symfony#51381
| License       | MIT
| Doc PR        | -

 * Deprecate `framework.validation.enable_annotations` in favor of `framework.validation.enable_attributes`
 * Deprecate `framework.serializer.enable_annotations` in favor of use `framework.serializer.enable_attributes`
 * Deprecate `ValidatorBuilder::enableAnnotationMapping()` in favor of `ValidatorBuilder::enableAttributeMapping()`
 * Deprecate `ValidatorBuilder::disableAnnotationMapping()` in favor of `ValidatorBuilder::disableAttributeMapping()`
 * Deprecate `AnnotationLoader` in favor of `AttributeLoader`

Commits
-------

2e1e805f89 [FrameworkBundle][Validator] Deprecate annotation occurrences
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this issue Aug 22, 2023
…rrences (alexandre-daubois)

This PR was merged into the 6.4 branch.

Discussion
----------

[FrameworkBundle][Validator] Deprecate annotation occurrences

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | Part of symfony/symfony#51381
| License       | MIT
| Doc PR        | -

 * Deprecate `framework.validation.enable_annotations` in favor of `framework.validation.enable_attributes`
 * Deprecate `framework.serializer.enable_annotations` in favor of use `framework.serializer.enable_attributes`
 * Deprecate `ValidatorBuilder::enableAnnotationMapping()` in favor of `ValidatorBuilder::enableAttributeMapping()`
 * Deprecate `ValidatorBuilder::disableAnnotationMapping()` in favor of `ValidatorBuilder::disableAttributeMapping()`
 * Deprecate `AnnotationLoader` in favor of `AttributeLoader`

Commits
-------

2e1e805f89 [FrameworkBundle][Validator] Deprecate annotation occurrences
@alexandre-daubois
Copy link
Contributor

alexandre-daubois commented Aug 23, 2023

nicolas-grekas added a commit that referenced this issue Oct 17, 2023
…andre-daubois)

This PR was merged into the 6.4 branch.

Discussion
----------

[FrameworkBundle][Routing] Deprecate annotations

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | Part of #51381
| License       | MIT
| Doc PR        | -

 * Deprecate the `routing.loader.annotation` service, use the `routing.loader.attribute` service instead
 * Deprecate the `routing.loader.annotation.directory` service, use the `routing.loader.attribute.directory` service instead
 * Deprecate the `routing.loader.annotation.file` service, use the `routing.loader.attribute.file` service instead
 * Deprecate `AnnotationClassLoader`, use `AttributeClassLoader` instead
 * Deprecate `AnnotationDirectoryLoader`, use `AttributeDirectoryLoader` instead
 * Deprecate `AnnotationFileLoader`, use `AttributeFileLoader` instead
 * Deprecate `AnnotatedRouteControllerLoader`, use `AttributeRouteControllerLoader`

Commits
-------

708b1b6 [FrameworkBundle][Routing] Deprecate annotations
nicolas-grekas added a commit that referenced this issue Oct 17, 2023
…lexandre-daubois)

This PR was merged into the 6.4 branch.

Discussion
----------

[FrameworkBundle][Serializer] Deprecate annotations

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | -
| License       | MIT
| Doc PR        | -

Part of #51381

Commits
-------

a658692 [FrameworkBundle][Serializer] Deprecate annotations
symfony-splitter pushed a commit to symfony/http-kernel that referenced this issue Oct 17, 2023
…lexandre-daubois)

This PR was merged into the 6.4 branch.

Discussion
----------

[FrameworkBundle][Serializer] Deprecate annotations

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | -
| License       | MIT
| Doc PR        | -

Part of symfony/symfony#51381

Commits
-------

a6586923e7 [FrameworkBundle][Serializer] Deprecate annotations
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this issue Oct 17, 2023
…lexandre-daubois)

This PR was merged into the 6.4 branch.

Discussion
----------

[FrameworkBundle][Serializer] Deprecate annotations

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | -
| License       | MIT
| Doc PR        | -

Part of symfony/symfony#51381

Commits
-------

a6586923e7 [FrameworkBundle][Serializer] Deprecate annotations
@xabbuh
Copy link
Member

xabbuh commented Oct 24, 2023

I've checked three more boxes. The Attribute namespace is the only thing left to do, right?

@alexandre-daubois
Copy link
Contributor

I'd say yes!

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 25, 2023

Let's close this issue then? I don't mind the namespace - or I would mind the migration cost :)
We can always reconsider in 7.x (0 < x < 4).

@xabbuh
Copy link
Member

xabbuh commented Oct 25, 2023

I agree

@xabbuh xabbuh closed this as completed Oct 25, 2023
@wouterj
Copy link
Member

wouterj commented Oct 25, 2023

I think this is a good option: "We could instead provide the alternative in 6.4, and deprecate in 7.1."

Let's alias in 6.4/7.0 and start the deprecation path in 7.x. By the time 8.0 is released, libraries only need to support 6.4 and 7.x, meaning they can switch to the Attribute namespace. Applications can, whenever they want in the next 2 years, do a small find and replace dance.

Sure, the "Annotation" part doesn't mind an experienced PHP and Symfony developer that knows the time before PHP attributes. But it can already confuse people that are learning Symfony right now, and this will increase in significance the more we are away of the doctrine/annotations era.

@wouterj
Copy link
Member

wouterj commented Oct 25, 2023

Here we go: #52294

@nicolas-grekas
Copy link
Member

Thanks you all for the help!

nicolas-grekas added a commit that referenced this issue Oct 30, 2023
…s (wouterj)

This PR was merged into the 6.4 branch.

Discussion
----------

[Routing][Serializer] Add annotation -> attribute aliases

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #51381
| License       | MIT

Commits
-------

cce20a4 Add annotation -> attribute aliases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants