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

[Documentation] Types Cleanup #6341

Closed
wants to merge 6 commits into from
Closed

[Documentation] Types Cleanup #6341

wants to merge 6 commits into from

Conversation

ThomasLandauer
Copy link
Contributor

@ThomasLandauer ThomasLandauer commented Mar 20, 2024

Page: https://www.doctrine-project.org/projects/doctrine-dbal/en/3.8/reference/types.html#reference

This PR is the "sibling" of doctrine/orm#11384

ThomasLandauer added a commit to ThomasLandauer/orm that referenced this pull request Mar 20, 2024
... in favor of https://www.doctrine-project.org/projects/doctrine-dbal/en/3.8/reference/types.html#reference

Page: https://www.doctrine-project.org/projects/doctrine-orm/en/2.19/reference/basic-mapping.html#doctrine-mapping-types

As announced in doctrine/dbal#6336 (comment) , the goal is to remove this duplicated type information from ORM and replace it with a link to DBAL.

In doctrine/dbal#6341 , I'm adding any detail which I'm deleting here to the DBAL.
ThomasLandauer added a commit to ThomasLandauer/orm that referenced this pull request Mar 20, 2024
... in favor of https://www.doctrine-project.org/projects/doctrine-dbal/en/3.8/reference/types.html#reference

Page: https://www.doctrine-project.org/projects/doctrine-orm/en/2.19/reference/basic-mapping.html#doctrine-mapping-types

As announced in doctrine/dbal#6336 (comment) , the goal is to remove this duplicated type information from ORM and replace it with a link to DBAL.

In doctrine/dbal#6341 , I'm adding any detail which I'm deleting here to the DBAL.
SenseException pushed a commit to doctrine/orm that referenced this pull request Mar 20, 2024
@SenseException
Copy link
Member

Please squash the commits like you did in doctrine/orm#11384

@ThomasLandauer
Copy link
Contributor Author

OK, done :-)

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Thank you for your attempt to improve the documentation. However, I don't believe any of those changes actually improve what we currently have. In most cases, you replace perfectly find statements with implementation details and technicalities.

docs/en/reference/types.rst Outdated Show resolved Hide resolved
docs/en/reference/types.rst Outdated Show resolved Hide resolved
docs/en/reference/types.rst Outdated Show resolved Hide resolved
@@ -369,7 +370,7 @@ real arrays or JSON format arrays.
array
^^^^^

Maps and converts array data based on PHP serialization.
Maps and converts array data using PHP's ``serialize()`` and ``unserialize()``.
Copy link
Member

Choose a reason for hiding this comment

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

Implementation details don't belong into a documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case it's an added benefit, cause if somebody has questions about some details, they can just find out what serialize() and unserialize() are doing exactly.

Copy link
Member

Choose a reason for hiding this comment

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

Don't overload the reader with technicalities in the first sentence then. Maybe just add a like to PHP docs at the end of this section for futher reading.

Copy link
Contributor Author

@ThomasLandauer ThomasLandauer Mar 21, 2024

Choose a reason for hiding this comment

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

Well, in this case the only thing that's really important is the deprecation ;-) So I moved it upwards now.

BTW: This link is not working: :ref:\json``, see https://www.doctrine-project.org/projects/doctrine-dbal/en/3.8/reference/types.html#array - do you know how to fix it? (There are more occurrences on the page)

Comment on lines -396 to +398
Maps and converts array data based on PHP comma delimited imploding and exploding.
Maps and converts array data using PHP's ``implode()`` and ``explode()``, with a comma as delimiter
(so only use this type if you are sure that your values cannot contain a ``,``).
Copy link
Member

Choose a reason for hiding this comment

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

Again: implementation details!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, the rather non-saying term "imploding and exploding" is repeated twice in the paragraph ;-)
Please leave the `implode()andexplode()`` in for now, so I don't forget it.
I will go over this page again (e.g. remove the phrase "Maps and converts" which is repeated all over), but I need to see it live - can't do this here on GitHub.
Besides, the main problem on this page IMO is the out-of-bounds "Mapping Matrix".

Copy link
Member

Choose a reason for hiding this comment

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

the note about not supporting values containing a comma is good to keep though.

ThomasLandauer and others added 3 commits March 21, 2024 15:18
@ThomasLandauer
Copy link
Contributor Author

All I did in this PR was adding some details that were present in the ORM docs, but were deleted in doctrine/orm#11384

docs/en/reference/types.rst Outdated Show resolved Hide resolved
Comment on lines -396 to +398
Maps and converts array data based on PHP comma delimited imploding and exploding.
Maps and converts array data using PHP's ``implode()`` and ``explode()``, with a comma as delimiter
(so only use this type if you are sure that your values cannot contain a ``,``).
Copy link
Member

Choose a reason for hiding this comment

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

the note about not supporting values containing a comma is good to keep though.

Comment on lines -426 to +428
Maps and converts array data based on PHP's JSON encoding functions.
Maps and converts array data using PHP's ``json_encode()`` and ``json_decode()``.
Copy link
Member

Choose a reason for hiding this comment

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

Implementation details, the current version is fine.

Comment on lines -466 to +468
Maps and converts object data based on PHP serialization.
Maps and converts object data using PHP's ``serialize()`` and ``unserialize()``.
Copy link
Member

Choose a reason for hiding this comment

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

Implementation details, the current version is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's take a look at the current version:

Maps and converts object data based on PHP serialization. If you need to store an exact representation of your object data, you should consider using this type as it uses serialization to represent an exact copy of your object as string in the database. Values retrieved from the database are always converted to PHP's object type using deserialization or null if no data is present.

The same pseudo-information ("serialization") is repeated over and over; but which form of serialization is hidden from the user. Why?
So ultimately, I'd suggest to add a link to https://www.php.net/manual/function.serialize.php - then half of this paragraph can be deleted.

Copy link
Member

Choose a reason for hiding this comment

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

which form of serialization is hidden from the user.

🤔 it says "PHP serialization"… are there several forms of PHP serialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know.
But if there's just one anyway, we can call it by its name; instead of leaving users uncertain.

Copy link
Member

Choose a reason for hiding this comment

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

I'm really not convinced.

Copy link
Member

Choose a reason for hiding this comment

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

It's like explaining how the motor of a car works while the user just wants to drive a car. If there are other places where the serialization got mentioned, then it's because we didn't know better about mentioning details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guys, I don't know what to say here. The real problem on that page is the horrible Mapping Matrix, but instead of discussing how it can be improved (i.e. removed), we keep arguing for three months if "PHP serialization" is a better term than serialize()?!

As I said, all I did was to copy it over from https://github.com/doctrine/orm/blame/3.0.x/docs/en/reference/basic-mapping.rst#L257, where it has been sitting for some 14 years - and nobody of you cared.

are there several forms of PHP serialization?

Searching for "serialize" on https://www.php.net/ gives me 28 hits.

So if everybody agrees with @derrabus that the current version is "perfectly fine", you can just close here...

Copy link
Member

@derrabus derrabus Jun 21, 2024

Choose a reason for hiding this comment

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

The real problem on that page is the horrible Mapping Matrix,

But that's not what this PR changed, is it.

but instead of discussing how it can be improved (i.e. removed), we keep arguing for three months if "PHP serialization" is a better term than serialize()?!

Right, we should've closed this PR sooner.

@greg0ire greg0ire closed this Jun 21, 2024
@ThomasLandauer ThomasLandauer deleted the patch-2 branch June 21, 2024 12:59
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.

5 participants