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

Migrate more documentation towards attributes #10157

Merged

Conversation

greg0ire
Copy link
Member

The previous rework missed some details / left the documentation in an inconsistent state. Searching for "annotation" case insensitively allows to find discrepancies and forgotten things.

@greg0ire
Copy link
Member Author

I also added another commit to address @stof 's comment

@@ -154,7 +154,7 @@ As you can see, we have a method "setBlockEntity" which ties a potential strateg
* This var contains the classname of the strategy
* that is used for this blockitem. (This string (!) value will be persisted by Doctrine ORM)
*
* This is a doctrine field, so make sure that you use an @column annotation or setup your
* This is a doctrine field, so make sure that you use an @[Column] attribute or setup your
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not be a @ here

@@ -53,20 +53,17 @@ code, enforcing it at any time is important so that customers with
a unknown reputation don't owe your business too much money.

We can enforce this constraint in any of the metadata drivers.
First Annotations:
First Attributes:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the doc below shows other formats. So maybe it should keep annotations as well.

And for Yaml, there is a sentence saying this will happen before Beta 1 though which looks totally wrong in the doc (and it does not say beta 1 of which version)

@@ -124,18 +125,18 @@ used in the examples. For information on the usage of the
AnnotationDriver, XmlDriver or YamlDriver please refer to the dedicated
chapters ``Annotation Reference``, ``XML Mapping`` and ``YAML Mapping``.

The annotation driver can be configured with a factory method on
The attribute driver can be configured with a factory method on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no factory method (and before, it was not on the Configuration class)

@@ -37,7 +37,28 @@ A many-to-one association is the most common association between objects. Exampl

.. configuration-block::

.. code-block:: php
.. code-block:: annotation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put attribute before annotation, as attribute is the default in the docs

@@ -437,7 +451,7 @@ Here is the list of possible generation strategies:
thus generated) by your code. The assignment must take place before
a new entity is passed to ``EntityManager#persist``. NONE is the
same as leaving off the @GeneratedValue entirely.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GeneratedValue should be replaced too, meaning the attribute instead

$config = ORMSetup::createAttributeMetadataConfiguration(
paths: array(__DIR__."/src"),
isDevMode: true,
);
// or if you prefer YAML or XML
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we have a or if you prefer annotation ?

$this->longitude = $longitude;
public function __construct(
private float $latitude,
private float$longitude,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private float$longitude,
private float $longitude,

@@ -154,7 +154,7 @@ As you can see, we have a method "setBlockEntity" which ties a potential strateg
* This var contains the classname of the strategy
* that is used for this blockitem. (This string (!) value will be persisted by Doctrine ORM)
*
* This is a doctrine field, so make sure that you use an @column annotation or setup your
* This is a doctrine field, so make sure that you use an @[Column] attribute or setup your
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* This is a doctrine field, so make sure that you use an @[Column] attribute or setup your
* This is a doctrine field, so make sure that you use an #[Column] attribute or setup your

#[Column(type: 'point')]
private Point $point;

#[Column()]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[Column()]
#[Column]

class UserListener
{
#[PrePersist]
public function prePersistHandler(User $user, LifecycleEventArgs $event) { // ... }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we make the return type explicit to indicate that we don't expect the listener to return anything?

Suggested change
public function prePersistHandler(User $user, LifecycleEventArgs $event) { // ... }
public function prePersistHandler(User $user, LifecycleEventArgs $event): void { // ... }

greg0ire and others added 2 commits October 21, 2022 12:31
The previous rework missed some details / left the documentation in an
inconsistent state. Searching for "annotation" case insensitively allows
to find discrepancies and forgotten things.
@greg0ire greg0ire force-pushed the followup-migrate-docs-to-attributes branch from e97c1f3 to 4e445fe Compare October 21, 2022 10:31
@derrabus derrabus added this to the 2.13.4 milestone Oct 21, 2022
@greg0ire greg0ire merged commit 9f926f0 into doctrine:2.13.x Oct 21, 2022
@greg0ire greg0ire deleted the followup-migrate-docs-to-attributes branch October 21, 2022 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants