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

[DOCS] Fix binary type format documentation #6700

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

jaapio
Copy link
Contributor

@jaapio jaapio commented Jan 10, 2025

The table contained an error which broke the parsing of the table. Row content cannot be at the same line as the row span definition.

The rendered table looks unstyled like this, I checked this directly with the phpdocumentor/guides project so no doctrine styling is included in the screenshot.

image

| [2] [6] | +--------------------------+ +----------------------------------------------------------+
| | | **SQL Server** | | ``BINARY(n)`` [4] |
| | +--------------------------+ +----------------------------------------------------------+
| | | **Oracle** | *all* | ``RAW(n)`` |
Copy link
Member

Choose a reason for hiding this comment

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

Previously, BINARY and RAW were grouped together (I don't know why though). Now that's no longer the case. Shouldn't the solution be to make the first cell bigger so that it lines up the ones in the "name" column?

@derrabus @morozov would you maybe know why there was this grouping / what rendering is the goal here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they need to be grouped we can simply remove the separator between the types.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the presentation of binary needs an update. I'd interpret the old style like SQL Server shares VARBINARY and BINARY, but RAW is afaik unknown to SQL Server.

Copy link
Member

Choose a reason for hiding this comment

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

I've never gotten into the details of this documentation but FWIW, RAW applies only to Oracle while BINARY and VARBINARY apply only to MySQL and SQL Server (see getBinaryTypeDeclarationSQLSnippet()).

It looks like we need to split the version column that spans three rows into two (one to span MySQL and SQL Server) and the other dedicated to Oracle. Then it should be accurate.

Copy link
Member

Choose a reason for hiding this comment

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

Also, quite likely, the PHP type for the BINARY data type is never resource (it was the case in older DBAL versions). Right now, it should support only string.

@jaapio jaapio requested a review from greg0ire January 28, 2025 18:45
@morozov
Copy link
Member

morozov commented Jan 28, 2025

Should this be squashed into one commit? I'm not sure if the commit history in the PR is helpful.

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

Squash would be nice

The table contained an error which broke the parsing of the table.
Row content cannot be at the same line as the row span definition.
@jaapio jaapio force-pushed the docs/fix-table-display branch from 2a03cfc to 61dcd2a Compare January 29, 2025 06:57
@jaapio
Copy link
Contributor Author

jaapio commented Jan 29, 2025

Squashed :-)

@greg0ire greg0ire added this to the 4.2.3 milestone Jan 29, 2025
@greg0ire greg0ire merged commit 77a6041 into doctrine:4.2.x Jan 29, 2025
1 check passed
@greg0ire
Copy link
Member

Thanks for fixing this super long standing issue!

@jaapio
Copy link
Contributor Author

jaapio commented Jan 29, 2025

Is there anything needed to get this at the website? You might want to upgrade the phpDocumentor/guides packages in the site so you get some extra fixes impacting tables.

@greg0ire
Copy link
Member

A cronjob should publish the new version overnight. Thanks for the fixes!

@greg0ire
Copy link
Member

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 this pull request may close these issues.

4 participants