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.

Sign up for GitHub

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

Jump to bottom

Add TokenType class #11228

Merged
merged 8 commits into from
Feb 7, 2024
Merged

Add TokenType class #11228

merged 8 commits into from
Feb 7, 2024

Conversation

connorhu
Copy link
Contributor

@connorhu connorhu commented Feb 6, 2024

The idea of a TokenType class was discussed to make it easier to support ORM2 and ORM3 together.
beberlei/DoctrineExtensions#421 (comment)

derrabus
derrabus reviewed Feb 6, 2024
View reviewed changes
src/Query/TokenType.php Outdated Show resolved Hide resolved
@derrabus
Copy link
Member

derrabus commented Feb 6, 2024

I suggest we have the old Lexer constants point to the new TokenType ones and flag the Lexer constants as @deprecated.

@connorhu
Copy link
Contributor Author

connorhu commented Feb 6, 2024

@derrabus Should I deprecate each constant or is one per section enough? I would mark all of them, but I don't know if you have any policy for that.

@derrabus
Copy link
Member

derrabus commented Feb 6, 2024

If we want static analyzers to pick up the deprecation, we need to flag each constant as deprecated.

@greg0ire
Copy link
Member

greg0ire commented Feb 6, 2024

I think you should search and replace occurrences of Lexer::T_ throughout the project (including in docs).

greg0ire
greg0ire previously approved these changes Feb 6, 2024
View reviewed changes
@greg0ire
Copy link
Member

greg0ire commented Feb 6, 2024

You will need to update this as well:

orm/psalm-baseline.xml

Lines 2047 to 2050 in 40fbbf4

<RedundantConditionGivenDocblockType>
<code>$AST instanceof AST\SelectStatement</code>
<code>$token === Lexer::T_IDENTIFIER</code>
</RedundantConditionGivenDocblockType>

Please take a look at this guide for more on how to try things locally.

greg0ire
greg0ire reviewed Feb 6, 2024
View reviewed changes
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Ah sorry I forgot: please add something in UPGRADE.md regarding the new deprecation.

@connorhu
Copy link
Contributor Author

connorhu commented Feb 7, 2024

@greg0ire I made the change you asked for.

greg0ire
greg0ire previously approved these changes Feb 7, 2024
View reviewed changes
@derrabus derrabus added this to the 2.19.0 milestone Feb 7, 2024
derrabus
derrabus requested changes Feb 7, 2024
View reviewed changes
docs/en/reference/dql-doctrine-query-language.rst Show resolved Hide resolved
src/Query/Lexer.php Outdated Show resolved Hide resolved
@connorhu
Copy link
Contributor Author

connorhu commented Feb 7, 2024

@derrabus I did this deprecation notation form based on the phpdoc in the ORMException file, which means it is in "wrong" format too. Should I fix it in a separate PR that file to use @see?

orm/src/ORMException.php

Line 18 in 40fbbf4

* @deprecated Use Doctrine\ORM\Exception\ORMException for catch and instanceof

derrabus
derrabus reviewed Feb 7, 2024
View reviewed changes
src/Query/Lexer.php Outdated Show resolved Hide resolved
@derrabus derrabus merged commit 5049b61 into doctrine:2.19.x Feb 7, 2024
20 checks passed
@connorhu
Copy link
Contributor Author

connorhu commented Feb 7, 2024

Thank you for your help in the creation of the PR and thank you for merging it.

derrabus added a commit that referenced this pull request Feb 7, 2024
@derrabus
Merge branch '2.19.x' into 3.1.x
9a24ce5 
* 2.19.x:
  Add TokenType class (#11228)
derrabus added a commit to derrabus/orm that referenced this pull request Feb 7, 2024
@derrabus
Merge branch '3.1.x' into 4.0.x
0917109 
* 3.1.x:
  Add TokenType class (doctrine#11228)
  Revert "Merge pull request doctrine#11229 from greg0ire/add-columns"
  Add columns for 3.1.x and 4.0x
  Update version ORM from 2 to 3 in docs (doctrine#11221)
  Clean up outdated sentence (doctrine#11224)
  Update README.md
  Point link to correct upgrade guide (doctrine#11220)
  Ignore subclasses without discriminatorValue when generating discriminator column condition SQL (doctrine#11200)
  Update branches in README
@derrabus derrabus mentioned this pull request Feb 7, 2024
Merged
@derrabus
Copy link
Member

derrabus commented Feb 7, 2024

We missed a couple of references: #11234

@connorhu
Copy link
Contributor Author

connorhu commented Feb 7, 2024

It didn't occur to me to run phpstan with phpstan-deprecation-rules rules. That would have caught these mistakes.

@derrabus
Copy link
Member

derrabus commented Feb 7, 2024

We have Psalm for that, but we apparently allow deprecated constants to be used inside the Lexer class. 🙈

stof
stof reviewed Mar 1, 2024
View reviewed changes
src/Query/TokenType.php
public const T_CLOSE_CURLY_BRACE = 19;

// All tokens that are identifiers or keywords that could be considered as identifiers should be >= 100
/** @deprecated No Replacement planned. */
Copy link
Member

Choose a reason for hiding this comment

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

why adding a deprecated one in TokenType ?

Copy link
Member

Choose a reason for hiding this comment

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

Consistency, mainly. We've moved all token constants over to the new class.

@connorhu connorhu deleted the feature/token-type-bridge branch March 1, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@derrabus derrabus derrabus approved these changes

@greg0ire greg0ire greg0ire left review comments

@SenseException SenseException Awaiting requested review from SenseException

Assignees
No one assigned
Labels
Deprecation Improvement
Projects
None yet
Milestone
2.19.0
Development

Successfully merging this pull request may close these issues.
4 participants
@connorhu @derrabus @greg0ire @stof