Allow array in supported_scopes option

[jdeniau]

Currently, if you have an important number of scopes, you must set them into a one-line config option, which come to be really hard to read:

fos_oauth_server:
      service:
          options:
              supported_scopes: scope1 scope2 scope3 scope4 scope5 scope6

This PR allow using an array:

fos_oauth_server:
      service:
          options:
              supported_scopes:
                - scope1
                - scope2
                - scope3
                - scope4
                - scope5
                - scope6

It still works with a string if you have a small number of scopes

fos_oauth_server:
      service:
          options:
              supported_scopes: scope1 scope2

[jdeniau]

marked as error by php-cs-fixed ¯_(ツ)_/¯

[dkarlovi]

I don't like this approach. I'd rather we switch to an array in the config (deprecating the string first, of course).

[jdeniau]

@dkarlovi : I made the move to an array definition. I had to change prototype('scalar') by prototype('variable') because I did not manage to make a scalar prototype except for the supported_scopes attribute. It may be more logical though because the options parameters are managed by the oauth2-php library directly, and the library might accept any options (accepted values are mixed by the way).

About the string deprecation vs array: it does now works with both, and I think it's a good idea to allow string if you only have quite a few scopes.
If you really want to enforce arrays, and deprecate the string parameter "the symfony way", I will be stucked like I said before with the configuration. (I can throw a deprecation in the FOSOauthServerExtension.php but it's not the best option as Symfony provide a way to deprecate configuration parameter)

[dkarlovi]

@jdeniau you're right, looking at this, there's no need to deprecate the string notation, array notation is just a DX improvement. This looks good to me as is. 👍

Can you just add a blurb to the docs about this option, please?

[jdeniau]

@dkarlovi of course 🤦‍♂️ ! It's done

[dkarlovi]

LGTM, but I just remembered (sorry to be a nag): do you think we should check for a space in individual array items?

So, for example

scopes:
- foo bar
- bar

will become (foo) (bar) (bar), and not (foo bar) (bar) as one might expect.

[jdeniau]

Hmmm... sorry I don't think I understand:

The only manipulation is an implode(' '), so no matter how you put your scopes, you can write:

- foo
- bar
- bar

foo bar bar

or

- foo bar
- bar

the output will still be: foo bar bar

[jdeniau]

That "could" be interesting by the way if you want to do something like:

scopes:
  - foo:write foo:read
  - bar:write bar:read
  - baz:write baz:read

But it's an edge case due to the implementation, we might not document that as it might change if the implementation change

[dkarlovi]

the output will still be: foo bar bar

That's the problem, I feel. We probably shouldn't allow a space in array items at all since it's a separator. The user might expect it to behave differently than it in the end will.

[jdeniau]

In that case I can throw a Symfony Config InvalidConfigurationException if one of the item contains a space or a tab

[dkarlovi]

We should do that and, other than that, I see no reason not to merge here. 👍

[jdeniau]

@dkarlovi done

[dkarlovi]

@jdeniau awesome work, TYVM! \o/