Doctrine Inflector 2.0 #90

jwage · 2018-06-08T21:33:52Z

Refactor everything.

Majkl578 · 2018-06-11T11:12:54Z

lib/Doctrine/Inflector/Inflect.php

+     */
+    public function addRules(string $type, array $rules, bool $reset = false) : void
+    {
+        switch ($type) {


Instead of having this, why don't we just have addSingularRule(s) and addPluralRule(s)?

Majkl578 · 2018-06-11T11:13:28Z

lib/Doctrine/Inflector/Inflect.php

+                return;
+        }
+
+        throw new InvalidArgumentException(sprintf('Invalid rule type specified "%s"', $type));


This would need to be a package exception: Doctrine\Inflector\Exception\InvalidType.

Majkl578 · 2018-06-11T11:14:20Z

lib/Doctrine/Inflector/Inflect.php

+     */
+    public function tableize(string $word) : string
+    {
+        return strtolower(preg_replace('~(?<=\\w)([A-Z])~', '_$1', $word));


This isn't unicode safe, needs to be mb_strtolower and use u regex modifier.

Majkl578 · 2018-06-11T11:15:08Z

lib/Doctrine/Inflector/InflectInterface.php

+
+namespace Doctrine\Inflector;
+
+interface InflectInterface


Should not be suffixed with Interface and class name imho shouldn't be a verb (what's wrong with Inflector?

Right now I have left the old Inflector class with the static methods and have maintained BC with the old interface proxying to the new one. Once I have the tests moved over to the new API I will remove the existing Inflector class and rename things.

Majkl578 · 2018-06-11T11:16:12Z

lib/Doctrine/Inflector/InflectInterface.php

+     * Clears Inflectors inflected value caches, and resets the inflection
+     * rules to the initial values.
+     */
+    public function reset() : void;


Should imho be removed. Resetting should be done by creating new instance. (Same for add* methods btw.)

Majkl578 · 2018-06-11T11:16:28Z

lib/Doctrine/Inflector/InflectInterface.php

+     * ### Usage:
+     *
+     * {{{
+     * Inflector::rules('plural', ['/^(inflect)or$/i' => '\1ables']);


Needs to be updated.

Majkl578 · 2018-06-11T11:17:50Z

lib/Doctrine/Inflector/InflectorService.php

+use function array_merge;
+use function is_array;
+
+abstract class InflectorService


Not really convinced about having this thing here. :/

Majkl578 · 2018-06-11T11:18:29Z

lib/Doctrine/Inflector/Pluralizer.php

+
+                return $this->cache['pluralize'][$word];
+            }
+        }


As Scrutinizer points out, this will implicitly return null which is incompatible with non-null return type.

Majkl578 · 2018-06-11T11:18:54Z

lib/Doctrine/Inflector/Rules/Uninflected.php

+     *
+     * @var string[]
+     */
+    private $uninflected = [


Majkl578 · 2018-06-11T11:20:25Z

lib/Doctrine/Inflector/InflectorService.php

+
+abstract class InflectorService
+{
+    /** @var mixed[] */


Can't we do this better? Splitting into smaller units maybe?

composer.json

@@ -18,18 +18,19 @@
    "require-dev": {
        "doctrine/coding-standard": "^4.0",
        "phpstan/phpstan": "^0.9.2",
+        "phpstan/phpstan-phpunit": "^0.9.4",


Majkl578 · 2018-06-15T16:51:28Z

lib/Doctrine/Inflector/Inflector.php

+    /**
+     * Converts a word into the format for a Doctrine table name. Converts 'ModelName' to 'model_name'.
+     */
+    public function tableize(string $word) : string


Given that tableize() + classify() + camelize() + ucwords() isn't really inflecting, should we add it to the Inflector itself? Maybe separate class(es) would be better, especially once we introduce an interface.

I am not sure honestly. The definition of inflection is broad enough to include the tableize, classify, etc. methods. If you follow that logic, you'd have a different class for each method. That is a pain for the end user to have to have multiple classes/services.

Majkl578 · 2018-06-15T16:52:37Z

lib/Doctrine/Inflector/Inflector.php

+     *
+     * @return string The string with all delimiter-separated words capitalized.
+     */
+    public function ucwords(string $string, string $delimiters = " \n\t\r\0\x0B-") : string


Any idea for a better name? How about capitalize()?

Majkl578 · 2018-06-15T16:54:14Z

lib/Doctrine/Inflector/Inflector.php

+class Inflector
+{
+    /** @var WordInflector */
+    private $singularInflector;


Let's just name it $singularizer.

Majkl578 · 2018-06-15T16:54:37Z

lib/Doctrine/Inflector/Inflector.php

+    private $singularInflector;
+
+    /** @var WordInflector */
+    private $pluralInflector;


Let's just name it $pluralizer.

Majkl578 · 2018-06-15T16:58:38Z

lib/Doctrine/Inflector/InflectorFactory.php

+use Doctrine\Inflector\Rules\Singular;
+use Doctrine\Inflector\Rules\Uninflected;
+
+class InflectorFactory


I still don't like something on this class. Probably all those nullable parameters.

How about splitting this into three fluent (immutable) factories instead?

(new SingularizerFactory()) ->withRules($customRules) ->withIrregular($irregular) ->build(); // returns RulesetInflector

Those not overriden with with*() would use defaults during build().

Same for createPluralRuleset() and createInflector().

I can provide PR for this later separately (so we don't complicate thing too much in this PR.)

Issue recorded here #94

Majkl578 · 2018-06-15T17:01:47Z

lib/Doctrine/Inflector/Rules/Rules.php

+    public function inflect(string $word) : string
+    {
+        foreach ($this->rules as $rule) {
+            if (preg_match($rule->getFrom(), $word)) {


must use !== 0

Majkl578 · 2018-06-15T17:02:35Z

lib/Doctrine/Inflector/Rules/Uninflected.php

+    private function getRegex() : string
+    {
+        if ($this->regex === null) {
+            $words = array_map(function (Word $word) {


return type missing

Majkl578 · 2018-06-15T17:04:00Z

lib/Doctrine/Inflector/Rules/Uninflected.php

+                return $word->getWord();
+            }, $this->words);
+
+            $this->regex = '(?:' . implode('|', $words) . ')';


Possible regexp injection here, each item in words must be properly escaped by preg_quote() with correct delimiter set as 2nd argument (possibly in the array_map() above).

Majkl578 · 2018-06-15T17:05:45Z

lib/Doctrine/Inflector/RulesetInflector.php

+            return $inflected;
+        }
+
+        if ($this->ruleset->getUninflected()->isUninflected($word)) {


Imho this check should be prioritized before attempting to inflect an irregular.

Majkl578 · 2018-06-15T17:06:29Z

lib/Doctrine/Inflector/Rules/Ruleset.php

+class Ruleset
+{
+    /** @var Rules */
+    private $rules;


Would it make sense to call this $regular (opposed to $irregular) instead?

Majkl578

Let's finish the rest in follow-ups.

teohhanhui · 2018-07-18T17:11:17Z

What's the upgrade path from 1.x to 2.0? Will there be a 1.x version which will help users migrate to 2.0?

jwage · 2018-07-18T17:14:39Z

@teohhanhui The upgrade path is like this:

$plural = Inflector::pluralize('base');

Would be changed to this:

$inflector = new Inflector(
    new CachedWordInflector(new RulesetInflector(
        English\Rules::getSingularRuleset()
    )),
    new CachedWordInflector(new RulesetInflector(
        English\Rules::getPluralRuleset()
    ))
);
$plural = $inflector->pluralize('base');

So basically we're just not using static methods anymore and you have to instantiate the Inflector. You can see the 2.0 docs here https://www.doctrine-project.org/projects/doctrine-inflector/en/2.0/

garrettw · 2018-07-18T18:00:04Z

not using static methods

English\Rules::getSingularRuleset()

😉

teohhanhui · 2018-07-18T18:20:00Z

That seems really verbose! Would you consider adding a static factory method to get the default inflector?

jwage · 2018-07-18T18:21:29Z

@garrettw I should clarify, we're not using static methods for things that don't make sense to be static. Returning a static set of rules is a normal use case for a static method.

@teohhanhui You would do that setup somewhere in your dependency injection container/layer and then you would just get the inflector from your container and inject it to the places that you need it.

garrettw · 2018-07-18T19:53:46Z

@jwage relax, I'm just messing with you 😉

But in all seriousness, it is pretty verbose. I do understand the whole DIC pattern but I could still see a small factory being useful in some cases.

jwage added the Improvement label Jun 8, 2018

jwage added this to the 2.0 milestone Jun 8, 2018

jwage self-assigned this Jun 8, 2018

jwage changed the title ~~Move Doctrine\Common\Inflector to Doctrine\Inflector.~~ Doctrine Inflector 2.0 Jun 8, 2018

Majkl578 reviewed Jun 11, 2018

View reviewed changes

jwage force-pushed the inflector2.0 branch 16 times, most recently from 7cf9038 to 8192c8a Compare June 13, 2018 00:47

jwage mentioned this pull request Jun 13, 2018

Add 2.0 version for doctrine/inflector project. doctrine/doctrine-website#174

Merged

jwage requested a review from Majkl578 June 13, 2018 04:09

jwage force-pushed the inflector2.0 branch 6 times, most recently from f202eeb to 4a0fe1b Compare June 13, 2018 17:09

carusogabriel suggested changes Jun 14, 2018

View reviewed changes

composer.json

@@ -18,18 +18,19 @@

"require-dev": {

"doctrine/coding-standard": "^4.0",

"phpstan/phpstan": "^0.9.2",

"phpstan/phpstan-phpunit": "^0.9.4",

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

jwage force-pushed the inflector2.0 branch from 4a0fe1b to 90b264f Compare June 14, 2018 01:09

carusogabriel approved these changes Jun 14, 2018

View reviewed changes

Majkl578 reviewed Jun 15, 2018

View reviewed changes

jwage force-pushed the inflector2.0 branch from 90b264f to 67bf059 Compare June 15, 2018 18:13

Doctrine Inflector 2.0

2b6ecbc

jwage force-pushed the inflector2.0 branch from 67bf059 to 2b6ecbc Compare June 15, 2018 18:24

Majkl578 approved these changes Jun 15, 2018

View reviewed changes

jwage merged commit f62dcf4 into master Jun 15, 2018

jwage deleted the inflector2.0 branch June 15, 2018 18:37

alcaeus modified the milestones: 2.0.0, 1.4.0 May 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Doctrine Inflector 2.0 #90

Doctrine Inflector 2.0 #90

jwage commented Jun 8, 2018 •

edited

Loading

Majkl578 Jun 11, 2018

Majkl578 Jun 11, 2018

Majkl578 Jun 11, 2018

Majkl578 Jun 11, 2018

jwage Jun 11, 2018

Majkl578 Jun 11, 2018

Majkl578 Jun 11, 2018

Majkl578 Jun 11, 2018

Majkl578 Jun 11, 2018

Majkl578 Jun 11, 2018

Majkl578 Jun 11, 2018

This comment was marked as resolved.

This comment was marked as resolved.

Majkl578 Jun 15, 2018

jwage Jun 15, 2018

Majkl578 Jun 15, 2018

Majkl578 Jun 15, 2018

Majkl578 Jun 15, 2018

Majkl578 Jun 15, 2018

jwage Jun 15, 2018

Majkl578 Jun 15, 2018

Majkl578 Jun 15, 2018

Majkl578 Jun 15, 2018

Majkl578 Jun 15, 2018

Majkl578 Jun 15, 2018

Majkl578 left a comment

teohhanhui commented Jul 18, 2018

jwage commented Jul 18, 2018

garrettw commented Jul 18, 2018 •

edited

Loading

teohhanhui commented Jul 18, 2018 •

edited

Loading

jwage commented Jul 18, 2018

garrettw commented Jul 18, 2018 •

edited

Loading

Doctrine Inflector 2.0 #90

Doctrine Inflector 2.0 #90

Conversation

jwage commented Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Majkl578 left a comment

Choose a reason for hiding this comment

teohhanhui commented Jul 18, 2018

jwage commented Jul 18, 2018

garrettw commented Jul 18, 2018 • edited Loading

teohhanhui commented Jul 18, 2018 • edited Loading

jwage commented Jul 18, 2018

garrettw commented Jul 18, 2018 • edited Loading

jwage commented Jun 8, 2018 •

edited

Loading

garrettw commented Jul 18, 2018 •

edited

Loading

teohhanhui commented Jul 18, 2018 •

edited

Loading

garrettw commented Jul 18, 2018 •

edited

Loading