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

Upgrade to doctrine/inflector 1.4 #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ksom
Copy link
Member

@ksom ksom commented Sep 3, 2020

No description provided.

@@ -17,7 +17,7 @@ public function classToTableName($className, $includePrefix = true)
$parts = explode('_', $table_name);
$last = array_pop($parts);

$last = Inflector::pluralize($last);
$last = InflectorFactory::create()->build()->pluralize($last);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we instantiate the Inflector once and cache a reference to it?
Maybe even inject it via DI if it's provided as a service?

Copy link
Member Author

@ksom ksom Sep 3, 2020

Choose a reason for hiding this comment

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

This is now done by caching. Symfony do not provide this service, and it has now its own Inflector. But I don't want to rely on it to keep this bundle only depends on doctrine.

composer.json Outdated
@@ -11,7 +11,11 @@
{
"name": "Mathieu Lemoine",
"email": "[email protected]"
},
{
"name":"Virgile Vivier"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a public email you would care to include here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added it.

composer.json Outdated
@@ -20,9 +24,9 @@
"require": {
"php": ">=5.3.3",
"doctrine/orm": "^2.3",
"doctrine/inflector": "^1.0"
"doctrine/inflector": "^1.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there no way to keep compatibility with 1.0-1.4?
Otherwise, this would mean a major version change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I did it.

I think I could be nice to do a major version with next PRs to add for example :

  • class property types from PHP7
  • argument types from PHP7

And remove the code related to keep the compatibility.

I think this bundle won't add new features since the goal is already handled and stable for a while.
So if a project didn't match new requirements it can use a previous version that match without loosing features.

In this way we keep the code up to date.

What do you think about it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. But I don't really have the time to handle it (i.e. make the necessary changes to change the version here and in composer.
It's also been a while since I released a composer package and I'm not sure I remember how to do it properly.

If you have the time (and inclination) to provide two PRs: first one to change the version number for the next release and then one with the PHP7-related improvements, that would be great.

Also:
PSA: If you ever feel like becoming maintainer, then, PLEASE, reach out privately. I would be more than happy to help provide a second life to this project. Any project in the wemakecustom organisation with an open-source licence is ready and eager for the transition ;).

(Sidenote: I'm surprised you don't already the commit-bit on these repos...)

@ksom ksom force-pushed the fix-deprecated-inflector-use branch from 033dbe4 to e02f095 Compare September 3, 2020 19:22
@ksom ksom force-pushed the fix-deprecated-inflector-use branch from e02f095 to c2ad63c Compare September 3, 2020 19:35
@ksom ksom force-pushed the fix-deprecated-inflector-use branch from c2ad63c to 72f8f79 Compare September 4, 2020 18:23
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.

2 participants