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

Switch to proxy field is a breaking change #22

Open
jamesmacwhite opened this issue Dec 25, 2024 · 7 comments
Open

Switch to proxy field is a breaking change #22

jamesmacwhite opened this issue Dec 25, 2024 · 7 comments

Comments

@jamesmacwhite
Copy link
Contributor

jamesmacwhite commented Dec 25, 2024

Describe the bug

Happy Christmas!

Since using dev-craft-5 for the latest changes, I got a stacktrace on a custom OAuth client am getting this stack trace. I believe it is due to 8673272

I can update our client, but on the next release it might be worth flagging in the change logs as a breaking change, as without modifying our custom OAuth integration it breaks viewing the client in the CP.

It is due to the update for the macro being used for proxy URL I believe.

TypeError: craft\web\twig\Extension::mergeFilter(): Argument #1 ($arr1) must be of type Traversable|array, null given, called in /srv/app/nttmcoll-api/htdocs/storage/runtime/compiled_templates/b6/b69f9ffe590a9cfaf29db8f45671557f.php on line 100
#44 /vendor/craftcms/cms/src/web/twig/Extension.php(1358): craft\web\twig\Extension::mergeFilter
#43 /storage/runtime/compiled_templates/b6/b69f9ffe590a9cfaf29db8f45671557f.php(100): __TwigTemplate_c8d7a2ec99a1874076742b85e33a2b72::{closure}
#42 /vendor/twig/twig/src/Extension/CoreExtension.php(1967): Twig\Extension\CoreExtension::captureOutput
#41 /storage/runtime/compiled_templates/b6/b69f9ffe590a9cfaf29db8f45671557f.php(91): __TwigTemplate_c8d7a2ec99a1874076742b85e33a2b72::macro_proxyField
#40 /vendor/twig/twig/src/Extension/CoreExtension.php(1309): Twig\Extension\CoreExtension::callMacro
#39 /storage/runtime/compiled_templates/aa/aac7462a1e01cb4054326315df46406c.php(63): __TwigTemplate_7049ee6d4e0fa3e9ec26492a931609da::doDisplay
#38 /vendor/twig/twig/src/Template.php(393): Twig\Template::yield
#37 /storage/runtime/compiled_templates/8a/8a82972ef9cbf2fe83a6ab5bddd743b0.php(43): __TwigTemplate_21003cc45e707e58bd1a587343185e7d::doDisplay
#36 /vendor/twig/twig/src/Template.php(393): Twig\Template::yield
#35 /vendor/twig/twig/src/Template.php(349): Twig\Template::display
#34 /vendor/twig/twig/src/Template.php(364): Twig\Template::render
#33 /vendor/twig/twig/src/TemplateWrapper.php(35): Twig\TemplateWrapper::render
#32 /vendor/twig/twig/src/Environment.php(306): Twig\Environment::render
#31 /vendor/craftcms/cms/src/web/View.php(544): craft\web\View::renderTemplate
#30 /modules/nottinghamcollegeapi/src/integrations/consume/MicrosoftGraph.php(63): modules\nottinghamcollegeapi\integrations\consume\MicrosoftGraph::getSettingsHtml
#29 /vendor/twig/twig/src/Extension/CoreExtension.php(1802): Twig\Extension\CoreExtension::getAttribute
#28 /vendor/craftcms/cms/src/helpers/Template.php(148): craft\helpers\Template::attribute
#27 /storage/runtime/compiled_templates/35/35c22eb99bc368d89a1f75e36c582ab6.php(256): __TwigTemplate_85bba21ad6a1600ed66d686b1e994e2e::block_content
#26 /vendor/twig/twig/src/Template.php(437): Twig\Template::yieldBlock
#25 /storage/runtime/compiled_templates/53/53ea1081c0cddcfae6b6e3952315c957.php(672): __TwigTemplate_702bb06319980d859b779874cf9433f8::block_main
#24 /vendor/twig/twig/src/Template.php(437): Twig\Template::yieldBlock
#23 /storage/runtime/compiled_templates/53/53ea1081c0cddcfae6b6e3952315c957.php(399): __TwigTemplate_702bb06319980d859b779874cf9433f8::block_body
#22 /vendor/twig/twig/src/Template.php(437): Twig\Template::yieldBlock
#21 /storage/runtime/compiled_templates/de/de8049a0dbab5f5758cdcc7c016f28ae.php(105): __TwigTemplate_97f2dd021a55f53729166d2d389a8199::doDisplay
#20 /vendor/twig/twig/src/Template.php(393): Twig\Template::yield
#19 /storage/runtime/compiled_templates/2d/2d7914e5242247f0099b7a70c14b3c6c.php(61): __TwigTemplate_ed27c44b50f33fb60a357c9a0b2daa88::doDisplay
#18 /vendor/twig/twig/src/Template.php(393): Twig\Template::yield
#17 /storage/runtime/compiled_templates/53/53ea1081c0cddcfae6b6e3952315c957.php(175): __TwigTemplate_702bb06319980d859b779874cf9433f8::doDisplay
#16 /vendor/twig/twig/src/Template.php(393): Twig\Template::yield
#15 /storage/runtime/compiled_templates/52/5254b8b787d043710b37e3de2663c495.php(57): __TwigTemplate_967310710d434970219beac8073d33c1::doDisplay
#14 /vendor/twig/twig/src/Template.php(393): Twig\Template::yield
#13 /storage/runtime/compiled_templates/35/35c22eb99bc368d89a1f75e36c582ab6.php(103): __TwigTemplate_85bba21ad6a1600ed66d686b1e994e2e::doDisplay
#12 /vendor/twig/twig/src/Template.php(393): Twig\Template::yield
#11 /vendor/twig/twig/src/Template.php(349): Twig\Template::display
#10 /vendor/twig/twig/src/Template.php(364): Twig\Template::render
#9 /vendor/twig/twig/src/TemplateWrapper.php(35): Twig\TemplateWrapper::render
#8 /vendor/twig/twig/src/Environment.php(306): Twig\Environment::render
#7 /vendor/craftcms/cms/src/web/View.php(544): craft\web\View::renderTemplate
#6 /vendor/craftcms/cms/src/web/View.php(597): craft\web\View::renderPageTemplate
#5 /vendor/craftcms/cms/src/web/TemplateResponseFormatter.php(57): craft\web\TemplateResponseFormatter::format
#4 /vendor/yiisoft/yii2/web/Response.php(1109): yii\web\Response::prepare
#3 /vendor/craftcms/cms/src/web/Response.php(341): craft\web\Response::prepare
#2 /vendor/yiisoft/yii2/web/Response.php(340): yii\web\Response::send
#1 /vendor/yiisoft/yii2/base/Application.php(390): yii\base\Application::run
#0 /index.php(12): null

Steps to reproduce

  1. Configure a custom OAuth integration class prior to commit: 8673272
  2. Run latest code changes as dev-craft-5
  3. craft\web\twig\Extension::mergeFilter(): Argument #1 ($arr1) must be of type Traversable|array, null given will occur when viewing the client CP settings

Craft CMS version

5.5.7

Plugin version

dev-craft-5

Multi-site?

No response

Additional context

No response

@engram-design
Copy link
Member

Ah, very sorry about that, I didn't consider the breaking change in that change. I'll see if I can amend that.

Would you be up for sharing your current custom client class?

@jamesmacwhite
Copy link
Contributor Author

Not a problem Josh, it is unreleased yet, so just wanted to highlight it.

Sure, our main integration class just extends the Microsoft Graph provider to modify the authorize/access endpoints as it must be pointed to our tenant ID and not the general Microsoft one.

All I need to do is modify our settings HTML part as that's where the issue is. The original code was based off the original integration class mainly.

<?php

/**
 * Nottingham College Module for Craft CMS 5.x
 *
 * @link      https://www.nottinghamcollege.ac.uk
 * @copyright Copyright (c) 2024 Nottingham College
 */

namespace modules\nottinghamcollegeapi\integrations\consume;

use Craft;
use verbb\consume\base\OAuthClient;
use modules\nottinghamcollegeapi\integrations\oauth\providers\MicrosoftGraph as MicrosoftGraphProvider;

/**
 * Consume Microsoft Graph OAuth Provider
 *
 * @author    Nottingham College
 * @package   NottinghamCollegeApi
 * @since     1.0.0
 *
 * @property-read null|string $primaryColor
 * @property-read null|string $icon
 * @property-read null|string $settingsHtml
 */
class MicrosoftGraph extends OAuthClient
{
    // Static Methods
    // =========================================================================

    public static function displayName(): string
    {
        return 'Microsoft Graph';
    }

    public static function getOAuthProviderClass(): string
    {
        return MicrosoftGraphProvider::class;
    }


    // Properties
    // =========================================================================

    public static string $providerHandle = 'Microsoft Graph';

    // Public Methods
    // =========================================================================

    public function getPrimaryColor(): ?string
    {
        return '#5e5e5e';
    }

    public function getIcon(): ?string
    {
        return '<svg fill="currentColor" viewBox="0 0 24 24"><path d="M0 0v11.408h11.408V0zm12.594 0v11.408H24V0zM0 12.594V24h11.408V12.594zm12.594 0V24H24V12.594z"></path></svg>';
    }

    public function getSettingsHtml(): ?string
    {
        return Craft::$app->getView()->renderTemplate('nottingham-college-api/consume/oauth/microsoft-graph', [
            'client' => $this,
        ]);
    }

}

Then our main provider class with the customisations needed on urlAuthorize and urlAuthorizeToken

<?php

/**
 * Nottingham College Module for Craft CMS 5.x
 *
 * @link      https://www.nottinghamcollege.ac.uk
 * @copyright Copyright (c) 2014 Nottingham College
 */

namespace modules\nottinghamcollegeapi\integrations\oauth\providers;

use League\OAuth2\Client\Token\AccessToken;
use verbb\auth\models\Token;
use verbb\auth\providers\Microsoft as MicrosoftProvider;

/**
 * Microsoft Graph OAuth Provider
 *
 * @author    Nottingham College
 * @package   NottinghamCollegeApi
 * @since     1.0.1
 */
class MicrosoftGraph extends MicrosoftProvider
{
    // Properties
    // =========================================================================

    /**
     * @var array|string[]
     */
    public array $defaultScopes = ['openid', 'email', 'profile'];

    /**
     * @var string
     */
    protected string $urlAuthorize = 'https://login.microsoftonline.com/{tenantId}/oauth2/v2.0/authorize';

    /**
     * @var string
     */
    protected string $urlAccessToken = 'https://login.microsoftonline.com/{tenantId}/oauth2/v2.0/token';

    /**
     * @var string
     */
    protected string $urlResourceOwnerDetails = 'https://graph.microsoft.com/v1.0/me';


    /**
     * @param Token|null $token
     * @return array
     */
    public function getApiRequestQueryParams(?Token $token): array
    {
        return [];
    }

    /**
     * @param $token
     * @return string[]
     */
    protected function getAuthorizationHeaders($token = null): array
    {
        return [
            'Authorization' => $token instanceof AccessToken ? sprintf('Bearer %s', $token->getToken()) : '',
        ];
    }

    /**
     * @param array $response
     * @param AccessToken $token
     * @return MicrosoftResourceOwner
     */
    protected function createResourceOwner(array $response, AccessToken $token): MicrosoftResourceOwner
    {
        return new MicrosoftResourceOwner($response);
    }

    /**
     * @return string
     */
    protected function getScopeSeparator(): string
    {
        return ' ';
    }
}

@jamesmacwhite
Copy link
Contributor Author

From quick testing the change is relatively simple I believe, just:

 public function getSettingsHtml(): ?string
    {
        $variables = $this->getSettingsHtmlVariables();
        return Craft::$app->getView()->renderTemplate('nottingham-college-api/consume/oauth/microsoft-graph', $variables);
    }

@engram-design
Copy link
Member

That should indeed be all you need to change, but just trying to replicate the error with your classes, but nothing yet...

In any case, I'll be sure to add a note, but it shouldn't be a breaking change.

@jamesmacwhite
Copy link
Contributor Author

jamesmacwhite commented Dec 25, 2024

Ah OK, that's odd, I had it happen on both dev and live straight away.

For reference, the contents of nottingham-college-api/consume/oauth/microsoft-graph is simply:

{% include 'consume/clients/oauth/_oauth' %}

@engram-design
Copy link
Member

Fixed for the next release. To get this early, run composer require verbb/consume:"dev-craft-5 as 2.0.1". But do continue to use getSettingsHtmlVariables() which is the recommended approach.

@jamesmacwhite
Copy link
Contributor Author

Thanks! I have updated our custom OAuth client to be compatible for 2.0.1 and above.

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

No branches or pull requests

2 participants