[String] Fix Notice when argument is empty string #39244

moldman · 2020-11-30T13:21:59Z

Q	A
Branch?	5.1
Bug fix?	yes
New feature?	no
Deprecations?	no
License	MIT

PHP Notice is generated when we pass empty string to singularize or pluralize method.

$inflector = new \Symfony\Component\String\Inflector\EnglishInflector();
$inflector->singularize('');

Notice: Uninitialized string offset: 0 in vendor/symfony/string/Inflector/EnglishInflector.php on line 344
PHP Notice:  Uninitialized string offset: 0 in vendor/symfony/string/Inflector/EnglishInflector.php on line 344

$inflector = new \Symfony\Component\String\Inflector\EnglishInflector();
$inflector->pluralize('');

Notice: Uninitialized string offset: 0 in vendor/symfony/string/Inflector/EnglishInflector.php on line 424
PHP Notice:  Uninitialized string offset: 0 in vendor/symfony/string/Inflector/EnglishInflector.php on line 424

Background:
When \Symfony\Component\PropertyAccess\PropertyAccessorInterface::setValue is used with _ property, then it calls \Symfony\Component\String\Inflector\EnglishInflector::singularize with empty string.

class Check
{
    public $_;
}
$check = new Check();
$pr = PropertyAccess::createPropertyAccessorBuilder()
    ->getPropertyAccessor();
if($pr->isWritable($check, '_')){
    $pr->setValue($check, '_', 'test');
}
var_dump($check);

Notice: Uninitialized string offset: 0 in vendor/symfony/string/Inflector/EnglishInflector.php on line 344
PHP Notice:  Uninitialized string offset: 0 in vendor/symfony/string/Inflector/EnglishInflector.php on line 344
...
Notice: Uninitialized string offset: 0 in vendor/symfony/string/Inflector/EnglishInflector.php on line 344
PHP Notice:  Uninitialized string offset: 0 in vendor/symfony/string/Inflector/EnglishInflector.php on line 344

Notice: Uninitialized string offset: 0 in vendor/symfony/string/Inflector/EnglishInflector.php on line 344
object(Check)#6 (1) {
  ["_"]=>
  string(4) "test"
}

P.S.
Another solution is to include empty string in \Symfony\Component\String\Inflector\EnglishInflector::$uninflected

    private static $uninflected = [
        '',
        'atad',
        'reed',
        'kcabdeef',
        'hsif',
        'ofni',
        'esoom',
        'seires',
        'peehs',
        'seiceps',
    ];

If this PR is not relevant please close and sorry for inconvenience.

carsonbot · 2020-11-30T13:22:04Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

derrabus · 2020-11-30T17:30:56Z

An empty string might not be valid input, but we must not trigger a notice in that case, imho.

Thank you very much for the PR. Can you provide a test for this change, please?

moldman · 2020-11-30T20:27:23Z

@derrabus Thank you for reviewing. I added tests, but now fabbot.io fails, not sure if I need to apply the proposed changes as soon as this code was not part of my changes.

derrabus · 2020-11-30T20:28:23Z

You can safely ignore fabbot here.

nicolas-grekas · 2020-11-30T22:17:28Z

I'd personally prefer adding the empty string to the $uninflected property.
Does this issue exist in the inflector component on 4.4 btw?

moldman · 2020-12-01T07:52:17Z

@nicolas-grekas I also like the solution with $uninflected property, but I think it may not be obvious why empty string is together with reversed words.

Tested now with 4.4, seems that issue exists:

\Symfony\Component\Inflector\Inflector::singularize('');

Notice: Uninitialized string offset: 0 in src/Symfony/Component/Inflector/Inflector.php on line 363
PHP Notice:  Uninitialized string offset: 0 in src/Symfony/Component/Inflector/Inflector.php on line 363
...
Notice: Uninitialized string offset: 0 in src/Symfony/Component/Inflector/Inflector.php on line 363
PHP Notice:  Uninitialized string offset: 0 in src/Symfony/Component/Inflector/Inflector.php on line 363

And PropertyAccesor have same behavior:

class Check
{
    public $_;
}
$check = new Check();
$pr = \Symfony\Component\PropertyAccess\PropertyAccess::createPropertyAccessorBuilder()
    ->getPropertyAccessor();

$pr->setValue($check, '_', 'test');

Notice: Uninitialized string offset: 0 in src/Symfony/Component/Inflector/Inflector.php on line 363
PHP Notice:  Uninitialized string offset: 0 in src/Symfony/Component/Inflector/Inflector.php on line 363
...
Notice: Uninitialized string offset: 0 in src/Symfony/Component/Inflector/Inflector.php on line 363
PHP Notice:  Uninitialized string offset: 0 in src/Symfony/Component/Inflector/Inflector.php on line 363

moldman · 2020-12-01T13:08:33Z

@nicolas-grekas Should i open another PR with fix for 4.4?

nicolas-grekas · 2020-12-01T14:49:22Z

it may not be obvious why empty string is together with reversed words.

I think the test cases are enough to remind about this.

Should i open another PR with fix for 4.4?

yes please!

moldman · 2020-12-01T15:40:46Z

@nicolas-grekas added to $uninflected

For 4.4: #39270

…man) This PR was merged into the 4.4 branch. Discussion ---------- [Inflector] Fix Notice when argument is empty string | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no  | Deprecations? | no  | License | MIT Fixing issue when we call `Inflector` with empty string: ``` \Symfony\Component\Inflector\Inflector::singularize(''); ``` ``` Notice: Uninitialized string offset: 0 in src/Symfony/Component/Inflector/Inflector.php on line 363 PHP Notice: Uninitialized string offset: 0 in src/Symfony/Component/Inflector/Inflector.php on line 363 ... Notice: Uninitialized string offset: 0 in src/Symfony/Component/Inflector/Inflector.php on line 363 PHP Notice: Uninitialized string offset: 0 in src/Symfony/Component/Inflector/Inflector.php on line 363 ``` Fix for 5.1 #39244 Commits ------- 2dfe342 [Inflector] Fix Notice when argument is empty string

derrabus · 2020-12-01T17:07:33Z

Good catch, thanks @moldman.

carsonbot added Status: Needs Review Inflector String Bug labels Nov 30, 2020

carsonbot changed the title ~~[Inflector][String] Fix Notice when argument is empty string~~ [Inflector] [String] Fix Notice when argument is empty string Nov 30, 2020

carsonbot added this to the 5.1 milestone Nov 30, 2020

moldman force-pushed the bugfix/inflector-empty-string branch from b9f26f8 to e9620ad Compare November 30, 2020 20:04

derrabus removed the Inflector label Nov 30, 2020

carsonbot changed the title ~~[Inflector] [String] Fix Notice when argument is empty string~~ [String] [Inflector] Fix Notice when argument is empty string Nov 30, 2020

derrabus approved these changes Nov 30, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 30, 2020

jderusse approved these changes Nov 30, 2020

View reviewed changes

[String] Fix Notice when argument is empty string

88c2b9b

moldman force-pushed the bugfix/inflector-empty-string branch from e9620ad to 88c2b9b Compare December 1, 2020 15:34

moldman mentioned this pull request Dec 1, 2020

[Inflector] Fix Notice when argument is empty string #39270

Merged

stof approved these changes Dec 1, 2020

View reviewed changes

derrabus changed the title ~~[String] [Inflector] Fix Notice when argument is empty string~~ [String] Fix Notice when argument is empty string Dec 1, 2020

derrabus merged commit 88cbcf9 into symfony:5.1 Dec 1, 2020

This was referenced Dec 18, 2020

Release v5.1.10 #39563

Merged

Release v5.2.1 #39564

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[String] Fix Notice when argument is empty string #39244

[String] Fix Notice when argument is empty string #39244

moldman commented Nov 30, 2020 •

edited

Loading

carsonbot commented Nov 30, 2020

derrabus commented Nov 30, 2020

moldman commented Nov 30, 2020

derrabus commented Nov 30, 2020

nicolas-grekas commented Nov 30, 2020 •

edited

Loading

moldman commented Dec 1, 2020

moldman commented Dec 1, 2020

nicolas-grekas commented Dec 1, 2020

moldman commented Dec 1, 2020

derrabus commented Dec 1, 2020

[String] Fix Notice when argument is empty string #39244

[String] Fix Notice when argument is empty string #39244

Conversation

moldman commented Nov 30, 2020 • edited Loading

carsonbot commented Nov 30, 2020

derrabus commented Nov 30, 2020

moldman commented Nov 30, 2020

derrabus commented Nov 30, 2020

nicolas-grekas commented Nov 30, 2020 • edited Loading

moldman commented Dec 1, 2020

moldman commented Dec 1, 2020

nicolas-grekas commented Dec 1, 2020

moldman commented Dec 1, 2020

derrabus commented Dec 1, 2020

moldman commented Nov 30, 2020 •

edited

Loading

nicolas-grekas commented Nov 30, 2020 •

edited

Loading