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.4 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

Hey!

I think @jschaedl has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

what should happen for a string containing both single quoted and double quoted strings but no other character ? Or for string containing single quotes and chars that cannot be kept unescaped in double-quoted strings ? for those 2 cases, the readability argument might still justify keeping single quotes. So maybe it should be implemented differently than putting the single quote in this regex

what should happen for a string containing both single quoted and double quoted strings but no other character ? Or for string containing single quotes and chars that cannot be kept unescaped in double-quoted strings ? for those 2 cases, the readability argument might still justify keeping single quotes. So maybe it should be implemented differently than putting the single quote in this regex

That's a valid point. I fiddled a bit with yamllint.com to see how their implementation works and it seems the algorithm for establishing what should be single- and double-quoted is a bit more complex. I don't speak Ruby, so I gave up on reading the code.

(sigh) At this point I'm thinking if just adding a new flag for forcing double-quoting on all strings would be easier. That would certainly make sense for some types of configuration files.

I don't speak Ruby, so I gave up on reading the code.

Can you link to that code ? I don't write Ruby, but I'm generally able to read it.

Can you link to that code ? I don't write Ruby, but I'm generally able to read it.

I think the source code for yamllint.com is here: https://github.com/gilltots/yamllint
They use this library for dumping YAML: https://github.com/afunai/ya2yaml

We could change this code:

to:

    case Escaper::requiresDoubleQuoting($value): 
        return Escaper::escapeWithDoubleQuotes($value); 
    case Escaper::requiresSingleQuoting($value): 
        $singleQuoted = Escaper::escapeWithSingleQuotes($value); 

        if (!str_contains($value, "'") {
             return $singleQuoted;
        }

        $doubleQuoted = Escaper::escapeWithDoubleQuotes($value); 

        return \strlen($doubleQuoted) < \strlen($singleQuoted) ? $doubleQuoted : $singleQuoted;
    case Parser::preg_match('{^[0-9]+[_0-9]*$}', $value): 
    case Parser::preg_match(self::getHexRegex(), $value): 
    case Parser::preg_match(self::getTimestampRegex(), $value): 
        return Escaper::escapeWithSingleQuotes($value); 

So if the string contains single quotes, the decision is made by resulting length of single quoted vs. double quoted string.

So if the string contains single quotes, the decision is made bei resulting length of single quoted vs. double quoted string.

Huh, that's smart. Thanks for the suggestion, I'll try to incorporate this into the code next week :)

(please rebase to remove the merge commit)

Yay, I think it works. I did my best trying to clean up git history, with moderate success.

@Ostrzyciel FYI, I have rebased and squashed the changes on your fork. :)

looks good to me, great work 👍

Thank you @Ostrzyciel.