Add Inflector component (from StringUtil of PropertyAccess) #18260

teohhanhui · 2016-03-22T09:41:41Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	no
Fixed tickets	N/A
License	MIT
Doc PR	N/A

As proposed by @dunglas here: #18166 (comment)

This will help us fix #18166

stof · 2016-03-22T09:46:41Z

src/Symfony/Component/Inflector/phpunit.xml.dist

+<?xml version="1.0" encoding="UTF-8"?>
+
+<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:noNamespaceSchemaLocation="http://schema.phpunit.de/4.1/phpunit.xsd"


should be current, not 4.1

Every other component uses 4.1 though. Perhaps this should be addressed in a separate PR (if it's an issue)?

stof · 2016-03-22T09:47:40Z

We need to keep some legacy test for the StringUtil class

teohhanhui · 2016-03-22T09:48:08Z

src/Symfony/Component/Inflector/LICENSE

@@ -0,0 +1,19 @@
+Copyright (c) 2012-2016 Fabien Potencier


The year is 2012 based on 7837f50

teohhanhui · 2016-03-22T09:53:01Z

@stof

We need to keep some legacy test for the StringUtil class

Should the test be duplicated?

stof · 2016-03-22T10:03:24Z

Should the test be duplicated?

no need to duplicate the whole tests (as the new component is tested). But we at least need a simple test, which would catch mistakes in the BC class

javiereguiluz · 2016-03-22T10:14:49Z

What about using Doctrine Inflector library instead of creating a new component?

teohhanhui · 2016-03-22T10:18:17Z

@javiereguiluz To me that'd be preferable too. But we need to use the same inflector for both PropertyAccess and PropertyInfo, according to @dunglas: #18166 (comment)

And if we switch both to the Doctrine Inflector, how should we handle the BC break?

linaori · 2016-03-22T11:37:26Z

I think it would be preferable for the community to use only 1 inflector library. Would it be an idea to use the doctrine variant instead and start patching that one to cover more use-cases when needed?

teohhanhui · 2016-03-22T11:40:11Z

How do we handle the BC break? Or would it not be considered a BC break? 😜

linaori · 2016-03-22T11:43:07Z

@teohhanhui I don't see why it would be. You can leave the deprecation as is and leave the tests as is. If the tests fail, you can patch those missing use-cases in the doctrine inflector. The only BC break would be tests failing because the doctrine inflector misses cases, but you will find out by hooking it up.

teohhanhui · 2016-03-22T11:45:25Z

@iltar I'm thinking BC breaks for applications due to different result of the singularization. It seems impossible for us to catch them all...

teohhanhui · 2016-03-22T11:46:26Z

But alas, it'd be for the best. What do you think, @dunglas?

javiereguiluz · 2016-03-22T12:33:07Z

I'm thinking BC breaks for applications due to different result of the singularization.

How could be any difference if both libraries supposedly do the same: transform English words into/from singular/plural?

stof · 2016-03-22T12:34:53Z

How could be any difference if both libraries supposedly do the same: transform English words into/from singular/plural?

Given that the conversion rules are quite complex, they may behave differently for some cases (common rules should work fine, but the component would be useless if the rule was as simple as removing the final s)

teohhanhui · 2016-03-22T12:41:16Z

If the application previously used the wrong singular form which matches what is produced by StringUtil::singularify, it'll stop working once we switch to Doctrine Inflector (given that the singular form is now correct); and vice versa, what used to be correct could now be wrong.

It's not really possible for us to ensure consistent before/after results. Now the question is whether this is part of our BC promise (of course, surprises like this would still be unwelcome and annoying for an application developer).

linaori · 2016-03-22T12:44:10Z

Luckily we have tests to verify the symfony variant still works, if they pass, no problem. If they fail: patch the inflector on doctrine's side.

stof · 2016-03-22T12:44:47Z

The compatibility test between both library would be as follow:

run the Symfony StringUtil testcases against the doctrine/inflector implementation
run the doctrine/inflector testcases against the StringUtil implementation (for relevant tests of course, as doctrine/inflector contains more methods than just singularify).

Test failures there would show us known incompatibilities (we may still have incompatibilities in things not covered by any of the testsuites). This would help us to make the decision.

teohhanhui · 2016-03-22T13:18:30Z

run the Symfony StringUtil testcases against the doctrine/inflector implementation

There are many failures: https://travis-ci.org/teohhanhui/symfony/jobs/117700871#L4206-L4791

And there's a bigger problem... Doctrine Inflector's singularize method doesn't support multiple singular forms (even though StringUtil mostly returned bogus results, but there are many valid cases for this...)

edit: Yes, there are still some valid use cases: https://www.quora.com/What-is-the-plural-of-axis

teohhanhui · 2016-03-22T15:53:55Z

After clean-up: https://travis-ci.org/teohhanhui/symfony/jobs/117721231#L4099-L4348

There are still a number of failures, and a few that we have no real way of fixing in the Doctrine Inflector (multiple words with the same plural form):

axes => axe, ax, axis
bases => base, basis

stof · 2016-03-22T16:59:52Z

There are still a number of failures, and a few that we have no real way of fixing in the Doctrine Inflector (multiple words with the same plural form):

And this is the difference between the doctrine inflector and the Symfony StringUtil: the doctrine inflector aims to provide a single singular string (which make sense as their primary usage of the inflector is generating method names during code generation, so they must settle on a single singular), while the goal of the StringUtil is to find all potential singular, to be able to call the method named by the developer with one of them.

and this difference of goal of the project is probably the main reason why we won't be able to switch to doctrine/inflector (as they won't change the focus of the Doctrine project just to allow us to use it)

teohhanhui · 2016-03-22T17:55:39Z

@stof It looks like I have to continue with this PR then...

fabpot · 2016-03-31T05:48:51Z

@teohhanhui Do you think you can finish (not sure what's left) this PR is the next couple of days? I'd like to get this in for 3.1 as it seems quite easy to acheive. Marking the whole component as being internal also gives us the opportunity to add more or even drop it if we find a better replacement in the meantime.

teohhanhui · 2016-03-31T06:21:26Z

What else do I need to do, other than adding @internal to the classes and methods?

teohhanhui · 2016-03-31T06:34:26Z

Marked as internal and added a disclaimer in the README

teohhanhui · 2016-03-31T06:42:15Z

Rebased.

fabpot · 2016-03-31T07:10:21Z

src/Symfony/Component/Inflector/README.md

@@ -0,0 +1,19 @@
+Inflector Component
+======================


Extra =s here

fabpot · 2016-03-31T07:12:36Z

👍 (after the minor comments have been fixed)

fabpot · 2016-03-31T07:32:13Z

src/Symfony/Component/PropertyAccess/StringUtil.php

 /**
 * Creates singulars from plurals.
 *
 * @author Bernhard Schussek <bschussek@gmail.com>
+ *
+ * @deprecated Deprecated since version 3.1, to be removed in 4.0. Use {@see \Doctrine\Common\Inflector\Inflector} instead.


Telling people to use Doctrine Inflector does not make sense here as the feature is not the same. And in fact, we are creating a new component exactly for this reason: we cannot use Doctrine inflector. So, the recommendation should be to use the new component instead.

dunglas · 2016-03-31T08:11:19Z

👍

fabpot · 2016-03-31T08:47:10Z

Thank you @teohhanhui.

@dunglas

…cess) (teohhanhui) This PR was merged into the 3.1-dev branch. Discussion ---------- Add Inflector component (from StringUtil of PropertyAccess) | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | no | Fixed tickets | N/A | License | MIT | Doc PR | N/A As proposed by @dunglas here: #18166 (comment) This will help us fix #18166 Commits ------- 8abebf6 Add Inflector component (from StringUtil of PropertyAccess)

flip111 · 2016-04-05T07:59:16Z

Is/can this component be used for #18149 ?

…glas) This PR was squashed before being merged into the 3.2-dev branch (closes #18337). Discussion ---------- [PropertyInfo] Support singular adder and remover | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18166 | License | MIT | Doc PR | n/a Fix #18166 with only a soft dependency to the PropertyAccess component. If #18260 is accepted, this PR can be reworked to use this new component, in the meantime it fixes the problem with a soft dependency. I don't now if it should be considered as a bug fix (and then merged in 2.8) or as a new feature. It's technically a bug fix but I'm not sure that introducing a new soft dependency is acceptable if older branches. What do you think @symfony/deciders? Commits ------- 4cbb60c [PropertyInfo] Support singular adder and remover

…class (xabbuh) This PR was merged into the 3.2 branch. Discussion ---------- [PropertyAccess] document deprecation of the StringUtil class | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18260 | License | MIT | Doc PR | Commits ------- f00c47a document deprecation of the StringUtil class

carsonbot added the Status: Needs Review label Mar 22, 2016

stof reviewed Mar 22, 2016
View reviewed changes

teohhanhui force-pushed the inflector-component branch from d85bcad to 9695e78 Compare March 22, 2016 09:47

teohhanhui reviewed Mar 22, 2016
View reviewed changes

teohhanhui force-pushed the inflector-component branch from 9695e78 to 11f5c30 Compare March 22, 2016 18:16

This was referenced Mar 28, 2016

[PropertyInfo] Support singular adder and remover #18337

Closed

[PropertyInfo] Singularize array mutator in ReflectionExtractor #18166

Closed

teohhanhui force-pushed the inflector-component branch from 11f5c30 to 8c78b2c Compare March 31, 2016 06:33

teohhanhui force-pushed the inflector-component branch 3 times, most recently from 8d9de9b to 10e2771 Compare March 31, 2016 06:42

teohhanhui force-pushed the inflector-component branch from 10e2771 to 0c69e72 Compare March 31, 2016 06:52

fabpot reviewed Mar 31, 2016
View reviewed changes

src/Symfony/Component/Inflector/README.md

@@ -0,0 +1,19 @@

Inflector Component

======================

Copy link

Member

fabpot Mar 31, 2016

Choose a reason for hiding this comment

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

Extra =s here

teohhanhui force-pushed the inflector-component branch from 0c69e72 to 9dbc0b4 Compare March 31, 2016 07:20

fabpot reviewed Mar 31, 2016
View reviewed changes

Add Inflector component (from StringUtil of PropertyAccess)

8abebf6

teohhanhui force-pushed the inflector-component branch from 9dbc0b4 to 8abebf6 Compare March 31, 2016 07:36

fabpot merged commit 8abebf6 into symfony:master Mar 31, 2016

teohhanhui deleted the inflector-component branch May 9, 2016 05:44

fabpot mentioned this pull request May 13, 2016

Release v3.1.0-BETA1 #18776

Merged

xabbuh mentioned this pull request May 21, 2017

[PropertyAccess] document deprecation of the StringUtil class #22824

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Inflector component (from StringUtil of PropertyAccess) #18260

Add Inflector component (from StringUtil of PropertyAccess) #18260

teohhanhui commented Mar 22, 2016

stof Mar 22, 2016

teohhanhui Mar 22, 2016

stof commented Mar 22, 2016

teohhanhui Mar 22, 2016

teohhanhui commented Mar 22, 2016

stof commented Mar 22, 2016

javiereguiluz commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

linaori commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

linaori commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

javiereguiluz commented Mar 22, 2016

stof commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

linaori commented Mar 22, 2016

stof commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

stof commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

fabpot commented Mar 31, 2016

teohhanhui commented Mar 31, 2016

teohhanhui commented Mar 31, 2016

teohhanhui commented Mar 31, 2016

fabpot Mar 31, 2016

fabpot commented Mar 31, 2016

fabpot Mar 31, 2016

dunglas commented Mar 31, 2016

fabpot commented Mar 31, 2016

flip111 commented Apr 5, 2016

Add Inflector component (from StringUtil of PropertyAccess) #18260

Add Inflector component (from StringUtil of PropertyAccess) #18260

Conversation

teohhanhui commented Mar 22, 2016

stof Mar 22, 2016

Choose a reason for hiding this comment

teohhanhui Mar 22, 2016

Choose a reason for hiding this comment

stof commented Mar 22, 2016

teohhanhui Mar 22, 2016

Choose a reason for hiding this comment

teohhanhui commented Mar 22, 2016

stof commented Mar 22, 2016

javiereguiluz commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

linaori commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

linaori commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

javiereguiluz commented Mar 22, 2016

stof commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

linaori commented Mar 22, 2016

stof commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

stof commented Mar 22, 2016

teohhanhui commented Mar 22, 2016

fabpot commented Mar 31, 2016

teohhanhui commented Mar 31, 2016

teohhanhui commented Mar 31, 2016

teohhanhui commented Mar 31, 2016

fabpot Mar 31, 2016

Choose a reason for hiding this comment

fabpot commented Mar 31, 2016

fabpot Mar 31, 2016

Choose a reason for hiding this comment

dunglas commented Mar 31, 2016

fabpot commented Mar 31, 2016

flip111 commented Apr 5, 2016