laminas-api-tools / api-tools-oauth2

Laminas module for implementing an OAuth2 server
https://api-tools.getlaminas.org/documentation
BSD 3-Clause "New" or "Revised" License
11 stars 15 forks source link

adds php 8.1 support #39

Closed jprangenbergde closed 2 years ago

jprangenbergde commented 2 years ago

adds php 8.1 support

jprangenbergde commented 2 years ago

@Ocramius Can you have a look? Have updated laminas-test to 4.0 (console got removed their). But api-tools-content-negotiation uses the console in the RequestFactory. -.-'

froschdesign commented 2 years ago

But api-tools-content-negotiation uses the console in the RequestFactory. -.-'

Only if it exists:

/**
 * @deprecated Since 1.6.0. This factory is no longer used within the module.
 */
class RequestFactory
{
    /**
     * @return ConsoleRequest|HttpRequest
     */
    public function __invoke(ContainerInterface $container)
    {
        // If console tooling is present, use that to determine whether or not
        // we are in a console environment. This approach allows overriding the
        // environment for purposes of testing HTTP requests from the CLI.
        if (class_exists(Console::class)) {
            return Console::isConsole() ? new ConsoleRequest() : new HttpRequest();
        }

        // If console tooling is not present, we use the PHP_SAPI value to decide.
        return PHP_SAPI === 'cli' ? new ConsoleRequest() : new HttpRequest();
    }
}

https://github.com/laminas-api-tools/api-tools-content-negotiation/blob/26434e97fc84e9554d79856b2b2c28740012a82d/src/Factory/RequestFactory.php#L16-L36

And the factory itself is no longer used.

jprangenbergde commented 2 years ago

But api-tools-content-negotiation uses the console in the RequestFactory. -.-'

Only if it exists:

/**
 * @deprecated Since 1.6.0. This factory is no longer used within the module.
 */
class RequestFactory
{
    /**
     * @return ConsoleRequest|HttpRequest
     */
    public function __invoke(ContainerInterface $container)
    {
        // If console tooling is present, use that to determine whether or not
        // we are in a console environment. This approach allows overriding the
        // environment for purposes of testing HTTP requests from the CLI.
        if (class_exists(Console::class)) {
            return Console::isConsole() ? new ConsoleRequest() : new HttpRequest();
        }

        // If console tooling is not present, we use the PHP_SAPI value to decide.
        return PHP_SAPI === 'cli' ? new ConsoleRequest() : new HttpRequest();
    }
}

https://github.com/laminas-api-tools/api-tools-content-negotiation/blob/26434e97fc84e9554d79856b2b2c28740012a82d/src/Factory/RequestFactory.php#L16-L36

And the factory itself is no longer used.

If PHP_SAPI === 'cli', then it is also possible.

`1) LaminasTest\ApiTools\OAuth2\Controller\AuthControllerTest::testToken Error: Class 'Laminas\Console\Request' not found

/github/workspace/vendor/laminas-api-tools/api-tools-content-negotiation/src/Factory/RequestFactory.php:29 /github/workspace/vendor/laminas/laminas-servicemanager/src/ServiceManager.php:645 /github/workspace/vendor/laminas/laminas-servicemanager/src/ServiceManager.php:218 /github/workspace/vendor/laminas/laminas-mvc/src/Service/ApplicationFactory.php:27 /github/workspace/vendor/laminas/laminas-servicemanager/src/ServiceManager.php:645 /github/workspace/vendor/laminas/laminas-servicemanager/src/ServiceManager.php:218 /github/workspace/vendor/laminas/laminas-mvc/src/Application.php:266 /github/workspace/vendor/laminas/laminas-test/src/PHPUnit/Controller/AbstractControllerTestCase.php:185 /github/workspace/test/Controller/AuthControllerTest.php:45 /github/workspace/test/Controller/AuthControllerTest.php:40 phpvfscomposer:///github/workspace/vendor/phpunit/phpunit/phpunit:60`

froschdesign commented 2 years ago

Then the DocBlock message is wrong: "This factory is no longer used within the module." 😞

froschdesign commented 2 years ago

@jprangenbergde Can you check where the factory is used?

jprangenbergde commented 2 years ago

ApplicationFactory

@froschdesign


namespace Laminas\Mvc\Service;

use Interop\Container\ContainerInterface;
use Laminas\Mvc\Application;
use Laminas\ServiceManager\Factory\FactoryInterface;

class ApplicationFactory implements FactoryInterface
{
    /**
     * Create the Application service
     *
     * Creates a Laminas\Mvc\Application service, passing it the configuration
     * service and the service manager instance.
     *
     * @param  ContainerInterface $container
     * @param  string $name
     * @param  null|array $options
     * @return Application
     */
    public function __invoke(ContainerInterface $container, $name, array $options = null)
    {
        return new Application(
            $container,
            $container->get('EventManager'),
            $container->get('Request'),
            $container->get('Response')
        );
    }
}

Mapped to:

'Request'  => 'Laminas\Mvc\Service\RequestFactory',

Called in:

vendor/laminas/laminas-test/src/PHPUnit/Controller/AbstractControllerTestCase.php:185
froschdesign commented 2 years ago

@jprangenbergde The error message you provided says something different:

LaminasTest\ApiTools\OAuth2\Controller\AuthControllerTest::testToken Error: Class 'Laminas\Console\Request' not found /github/workspace/vendor/laminas-api-tools/api-tools-content-negotiation/src/Factory/RequestFactory.php:29

https://github.com/laminas-api-tools/api-tools-oauth2/pull/39#issuecomment-1019954577

Laminas\ApiTools\ContentNegotiation\Factory\RequestFactory vs. Laminas\Mvc\Service\RequestFactory

So the question still remains: Why is the deprecated RequestFactory of api-tools-content-negotiation used here? Or did I miss something?

Ocramius commented 2 years ago

@jprangenbergde suggestion: add {"config": {"platform": {"php": "7.4.99"}}} to composer.json to prevent invalid dependency upgrades

trylika commented 2 years ago

what is missing to fix this PR? only composer.json platform config?

Ocramius commented 2 years ago

@trylika yes, and adding config.platform.php = 7.4.99 to composer.json, so that future composer update operations performed by contributors lead to valid dependency upgrades.

CI failures also may need to be checked

snapshotpl commented 2 years ago

Problem with this PR is that bshaffer/oauth2-server-php dependency doesn't support PHP 8.1 (and is not released for 3 years).

https://github.com/laminas-api-tools/api-tools-oauth2/runs/4920018102?check_suite_focus=true#step:3:480

Ocramius commented 2 years ago

Is @bshaffer still active overall?

bshaffer commented 2 years ago

Thank you for your concern @Ocramius! I am still alive and well! Although I haven't taken a look at that particular repo in a while. Happy to try and upgrade it for 8.1 if necessary!

Ocramius commented 2 years ago

Then I'd redirect contributors here to help you over there 😁