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

Use non-deprecated version of Lexer and Inflector #7274

Merged
merged 1 commit into from
Jun 25, 2018

Conversation

Majkl578
Copy link
Contributor

As of doctrine/common#845, Doctrine\Common\Lexer and Doctrine\Common\Util\Inflector trigger silent deprecations.

@Majkl578 Majkl578 added the Bug label Jun 25, 2018
@Majkl578 Majkl578 added this to the 2.6.2 milestone Jun 25, 2018
@Majkl578 Majkl578 requested a review from Ocramius June 25, 2018 12:23
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Majkl578 any composer.json changes required?

@Majkl578
Copy link
Contributor Author

Not necessarily, we still depend on Common which requires both of the packages. :) Do we want them explicitly listed here?

(Will address master later separately.)

@lcobucci
Copy link
Member

Not necessarily, we still depend on Common which requires both of the packages. :) Do we want them explicitly listed here?

@Majkl578 as long we don't plan to modify common and break everything is good HAHAHAH

Let's get it merged!

@lcobucci lcobucci merged commit ceda5d3 into doctrine:2.6 Jun 25, 2018
@lcobucci lcobucci self-assigned this Jun 25, 2018
@Majkl578 Majkl578 deleted the non-deprecated-lexer-and-inflector branch June 25, 2018 23:14
@stof
Copy link
Member

stof commented Jun 29, 2018

@lcobucci we should indeed add direct requirements on these packages though. but this could be done as part of #7249 instead.

@stof
Copy link
Member

stof commented Jun 29, 2018

@lcobucci this fix also needs to be applied in master btw, not only in 2.6

@Majkl578
Copy link
Contributor Author

we should indeed add direct requirements on these packages though

We don't need to do this now in 2.6 / 2.x imho since doctrine/common 2.x will always ship them.

this fix also needs to be applied in master btw

As said above:

(Will address master later separately.)

Master will be updated to not depend on doctrine/common, only separate packages. I'll likely cherry-pick this into #7249.
The tricky part is PropertyChangedListener + NotifyPropertyChanged, until it's decided what do do with those, we can't simply drop common yet. Maybe temporarily disabling the feature in master would be sufficient short-term.

@stof
Copy link
Member

stof commented Jun 29, 2018

@Majkl578 addressing it in master is exactly about applying this commit + adding the explicit requirements (once you drop the dependency). I think applying this part already makes sense to avoid the deprecation warnings (especially given that the work on #7249 has not started yet).

@stof
Copy link
Member

stof commented Jun 29, 2018

and even if you don't apply it to master for now, 2.7 needs it though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants