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

[5.8] BUG: Fix quoted environment variable parsing #27691

Merged
merged 4 commits into from
Feb 27, 2019
Merged

[5.8] BUG: Fix quoted environment variable parsing #27691

merged 4 commits into from
Feb 27, 2019

Conversation

mpyw
Copy link
Contributor

@mpyw mpyw commented Feb 27, 2019

For specifying NullBroadcastDriver in Laravel 5.7, we need to explicitly quote value like this:

BROADCAST_DRIVER='"null"'
<env name="BROADCAST_DRIVER" value='"null"' />

In Laravel 5.8, I got this error:

2019-02-27 14 50 34

According to @GrahamCampbell's commit #27462:

3. phpdotenv already natively handles quoted variables, so I don't see the need to strip quotes off again. This is only going to be annoying if you'd escaped the quotes in the env file, then laravel strips then anyway.

I think phpdotenv can natively handle quoted values, however, NullBroadcastDriver expects PHP string string(4) "null" on configuration, not string(6) ""null"" or string(6) "'null'".

@mpyw mpyw changed the title [5.8] Fix quoted environment variable parsing [5.8] BUG: Fix quoted environment variable parsing Feb 27, 2019
@mpyw
Copy link
Contributor Author

mpyw commented Feb 27, 2019

Workaround in config/broadcasting.php

<?php

return [

    /* ... */

    'connections' => [

        /* ... */

        'null' => [
            'driver' => 'null',
        ],

        // FIXME: delete me when https://github.com/laravel/framework/pull/27691 merged
        '"null"' => [
            'driver' => 'null',
        ],

    ],

];

@mpyw
Copy link
Contributor Author

mpyw commented Feb 27, 2019

I also propose adding single quotes support:

BROADCAST_DRIVER="'null'"
<env name="BROADCAST_DRIVER" value="'null'" />

if (($valueLength = strlen($value)) > 1 && $value[0] === '"' && $value[$valueLength - 1] === '"') {
    return substr($value, 1, -1);
}

if (($valueLength = strlen($value)) > 1 && ($value[0] === '"' && $value[$valueLength - 1] === '"' || $value[0] === "'" && $value[$valueLength - 1] === "'")) {
    return substr($value, 1, -1);
}

@sisve
Copy link
Contributor

sisve commented Feb 27, 2019

Is this something you could add a test for?

@mpyw
Copy link
Contributor Author

mpyw commented Feb 27, 2019

Added test.
(887916b is optional, feel free to pick or drop this)

@taylorotwell
Copy link
Member

@GrahamCampbell need your feedback here.

@taylorotwell
Copy link
Member

taylorotwell commented Feb 27, 2019

Why did you not keep the same stripping logic we had before? Why did you change to this regular expression? The logic appears a bit different in that it only checks if it begins with quotes and doesn't check if it ends in a quote?

@mpyw
Copy link
Contributor Author

mpyw commented Feb 27, 2019

@taylorotwell \1 means that

@taylorotwell taylorotwell merged commit af95c66 into laravel:5.8 Feb 27, 2019
@taylorotwell
Copy link
Member

I think going forward into future Laravel versions we may want to consider offering an alternative to "null" for these drivers. Even if we still support null, having another name that doesn't have this kinds of problems would be nice.

@mpyw mpyw deleted the fix-quoted-environment-variable branch February 27, 2019 14:04
@Donkfather
Copy link

how about 'none' or.. 'nullish' :)

@GrahamCampbell
Copy link
Member

This is not broken.

@GrahamCampbell
Copy link
Member

You should not have both single and double quotes.

@GrahamCampbell
Copy link
Member

@mpyw You should instead have:

BROADCAST_DRIVER="null"

or

BROADCAST_DRIVER='null'

Do not use both quotes.

@mpyw
Copy link
Contributor Author

mpyw commented Feb 27, 2019

  • NoneBroadcaster
  • NullishBroadcaster
  • NilBroadcaster
  • MuBroadcaster (like Perl 6 Mu)
  • FakeBroadcaster
  • DummyBroadcaster

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants