The scopes problem

[cmfcmf]

I still see scopes as a problematic point. If I have to look up the scopes needed for certain information in the provider's documentation, then it's pretty easy to look up the name of the field containing the required information too. And looking up stuff in the provider's documentation is exactly what this library should try to handle for the user, no?

[lmammino]

I believe the only solution would be to add a new method getFieldScopesMap that returns an array where FIELD constants are keys and SCOPE constants (from lusitanian lib) are values.

Obviously this would be an optional method available only on extractors for OAuth2 services who supports scopes.

We could also create a specific interface such as ScopeAwareExtractor that would define the signature of the getFieldScopesMap.

What do you think about it?

Can you help me with the development of this feature?

[cmfcmf]

I can try something out tomorrow. Your blog post and extended documentation are really helpful 👍

[lmammino]

@cmfcmf, nice to know you found it useful 😉

[cmfcmf]

/**
 * Interface ScopeAwareExtractorInterface
 *
 * @package OAuth\UserData\Extractor
 */
interface ScopeAwareExtractorInterface
{
    /**
     * Get a map of fields mapped to the required scopes.
     *
     * @return array
     */
    public function getFieldScopesMap();
}

That's what I have now, but the Interface might not be enough. I imagine a method being available for each ScopeAwareExtractor which looks like this, but don't know where to place it.

    /**
     * Returns all scopes needed for the specified fields.
     * @param  mixed $argument, ... All the fields you need to fetch.
     *
     * @return array All the scopes needed for the required fields.
     */
    public function getScopesForFields()
    {
        $fields = func_get_args();
        $scopesNeeded = array();
        $fieldScopesMap = $this->getFieldScopesMap();
        foreach ($fields as $field) {
            if ($fieldScopesMap[$field] !== null && !in_array($fieldScopesMap[$field], $scopesNeeded)) {
                $scopesNeeded[] = $fieldScopesMap[$field];
            }
        }

        return $scopesNeeded;
    }

[lmammino]

Great idea. I really like the method getScopesForFields. It really makes sense!

I am thinking about where to place it: I feel it would be something that should be implemented as a Trait, but, obviously, I don't think it's a good idea to raise the minimum PHP version to 5.4.

If we move it to the Extractor class (or the LazyExtractor class) we forces each extractor to be a ScopeAwareExtractorInterface and that's not good too (think of OAuth1 extractors). I know we should return an empty array for OAuth1 extractors but in my opinion it does not sound as an elegant solution...

We can even think about creating two new classes ScopeAwareExtractor (that extends Extractor) and ScopeAwareLazyExtractor (that extends LazyExtractor) but we will end up with some duplicate code among the two...

Another solution, maybe the most viable, could be to create a generic static function in the ArrayUtils class such as:

public static function extractMappedValues($map, $fields, $removeDuplicates = true)
{
   //...
}

then the function getScopesForFields() can be simplified:

public function getScopesForFields()
{
    return ArrayUtils::extractMappedValues($this->getFieldScopesMap(), func_get_args());
}

This would reduce a lot the code duplication issue and we can move the method getScopesForFields inside the ScopeAwareExtractorInterface then we could easily create the ScopeAwareExtractor and ScopeAwareLazyExtractor but probably this wouldn't be so necessary.

[cmfcmf]

Uhh, we're running into a chicken-egg problem: The place where scopes are passed to the service is right when instantiating the service:

$gitHub = $serviceFactory->createService('GitHub', $credentials, $storage, array('user'));

— https://github.com/Lusitanian/PHPoAuthLib/blob/master/examples/github.php#L35

But to instantiate the Extractor (which would have to be done before creating the service, to pass the scopes), the service must've already been created:

$extractorFactory = new \OAuth\UserData\ExtractorFactory();
$extractor = $extractorFactory->get($service);

[lmammino]

Yep... I was just thinking about this possible issue and considering if the scope related methods should be static...

$gitHub = $serviceFactory->createService(
    'GitHub', 
    $credentials, 
    $storage, 
    Github::getScopesForFields(
        Github::FIELD_USERNAME, 
        Github::FIELD_UNIQUE_ID
    )
);

[cmfcmf]

Hmm, this doesn't help much in my use case. I'm doing it like so now:

        // echo $this->getOAuthServiceName()
        // can be 'Twitter', 'GitHub', etc.
        $serviceStub = $serviceFactory->createService($this->getOAuthServiceName(), $credentials, $storage);

        $extractorFactory = new ExtractorFactory();
        $extractor = $extractorFactory->get($serviceStub);
        unset($serviceStub);

        if ($extractor instanceof ScopeAwareExtractorInterface && $extractor instanceof LazyExtractor) {
            if (!$extractor->supportsUniqueId()) {
                throw new \RuntimeException('Every provider must support a unique id!');
            }

            $fields = array($extractor::FIELD_UNIQUE_ID);
            if ($forRegistration) {
                /** @todo Language */
                if ($extractor->supportsUsername()) {
                    $fields[] = $extractor::FIELD_USERNAME;
                }
                if ($extractor->supportsEmail()) {
                    $fields[] = $extractor::FIELD_EMAIL;
                }
                if ($extractor->supportsVerifiedEmail()) {
                    $fields[] = $extractor::FIELD_VERIFIED_EMAIL;
                }
            }
            $scopes = $extractor->getScopesForFields($fields);
        } else {
            $scopes = array();
        }

        $this->service = $serviceFactory->createService($this->getOAuthServiceName(), $credentials, $storage, $scopes);

        return $this->service;

As you see, I need to call the supportsXXX() methods on the extractor before creating the service to check which fields I can requests an for which fields I have to calculate scopes for. So I ended up by creating the service two times.

I'll open a work in progress PR to let you see the changes instead of me only explaining what I did.

[lmammino]

Yes, I see we should find a way to simplify the interface and reduce the amount of code needed to perform what you're trying to do (that's a common use case IMHO).

Thanks for the effort. I'll follow your work on the fork.