Updated normalizeValue

[joshuabaker]

Fixes #74

[joshuabaker]

@reganlawton Please can you review this.

One key things that I’ve dropped is $this->oembed. I couldn’t understand why this was included when it’s absent in the first two returns of your code.

The second ArrayHelper::getValue call is there to avoid the error we’re seeing in #74 and should be kept for all sites that might have incorrectly saved the nested value.

[reganlawton]

Hi @joshuabaker thanks for the PR I'm been / currently recovering from a stroke, so I've been MIA lately and great for your code and only just starting to get back in too things myself.

The $this->oembed might have been older code to speed performances and my only thinking will be to ask if your able to change your code works with caching.

If you can change that I'll be happy to merge and what ask that you update the changelog and composer.json version. I'd be happy to have you to add a contribution note on README and/ or CHANGELOG file

[joshuabaker]

I'm been / currently recovering from a stroke

Sorry to hear that and wishing you a speedy, full recovery!

if your able to change your code works with caching

Was the $this->oembed doing that specifically? My sense is that caching is a separate concern and should probably be tackled separately; Likely in the service itself. Happy to discuss.

[denisyilmaz]

I'm been / currently recovering from a stroke

same from me! hope you get well soon.

@joshuabaker
I tested your updated OembedField.php in my project that has the issue we were discussing here #74
It seems it is missing the helpers in the head of the file. After adding these lines it works:

use craft\helpers\Json;
use craft\helpers\ArrayHelper;
use craft\helpers\UrlHelper;

My NEO field with oembed is now working with multiple langues thanks to your fix.
But: it need to add the oembed field block again to the entry to solve the issue. existing oembed blocks fail with:

{
  "debugMessage": "Argument 1 passed to craft\\helpers\\UrlHelper::isFullUrl() must be of the type string, array given, called in /home/www/vendor/wrav/oembed/src/fields/OembedField.php on line 133",
  "message": "Internal server error",
  "extensions": {
    "category": "internal"
  },
  "file": "/home/www/vendor/craftcms/cms/src/helpers/UrlHelper.php",
  "line": 61,
  "trace": [
    {
      "file": "/home/www/vendor/wrav/oembed/src/fields/OembedField.php",
      "line": 133,
      "call": "craft\\helpers\\UrlHelper::isFullUrl()"
    },
    {
      "file": "/home/www/vendor/craftcms/cms/src/base/Element.php",
      "line": 3490,
      "call": "wrav\\oembed\\fields\\OembedField::normalizeValue()"
    },
    {
      "file": "/home/www/vendor/craftcms/cms/src/base/Element.php",
      "line": 2788,
      "call": "craft\\base\\Element::normalizeFieldValue()"
    },
    {
      "file": "/home/www/vendor/craftcms/cms/src/base/Element.php",
      "line": 1530,
      "call": "craft\\base\\Element::getFieldValue()"
    },
    {
      "file": "/home/www/vendor/spicyweb/craft-neo/src/gql/types/elements/Block.php",
      "line": 76,
      "call": "craft\\base\\Element::__get()"
    },
    {
      "file": "/home/www/vendor/craftcms/cms/src/gql/base/ObjectType.php",
      "line": 47,
      "call": "benf\\neo\\gql\\types\\elements\\Block::resolve()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 624,
      "call": "craft\\gql\\base\\ObjectType::resolveWithDirectives()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 550,
      "call": "GraphQL\\Executor\\ReferenceExecutor::resolveFieldValueOrError()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 1196,
      "call": "GraphQL\\Executor\\ReferenceExecutor::resolveField()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 1146,
      "call": "GraphQL\\Executor\\ReferenceExecutor::executeFields()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 1107,
      "call": "GraphQL\\Executor\\ReferenceExecutor::collectAndExecuteSubfields()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 975,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeObjectValue()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 790,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeAbstractValue()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 655,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeValue()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 888,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeValueCatchingError()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 762,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeListValue()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 655,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeValue()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 557,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeValueCatchingError()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 1196,
      "call": "GraphQL\\Executor\\ReferenceExecutor::resolveField()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 1146,
      "call": "GraphQL\\Executor\\ReferenceExecutor::executeFields()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 1107,
      "call": "GraphQL\\Executor\\ReferenceExecutor::collectAndExecuteSubfields()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 975,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeObjectValue()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 790,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeAbstractValue()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 655,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeValue()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 888,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeValueCatchingError()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 762,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeListValue()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 655,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeValue()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 557,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeValueCatchingError()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 1196,
      "call": "GraphQL\\Executor\\ReferenceExecutor::resolveField()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 1146,
      "call": "GraphQL\\Executor\\ReferenceExecutor::executeFields()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 1107,
      "call": "GraphQL\\Executor\\ReferenceExecutor::collectAndExecuteSubfields()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 975,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeObjectValue()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 790,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeAbstractValue()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 655,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeValue()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 557,
      "call": "GraphQL\\Executor\\ReferenceExecutor::completeValueCatchingError()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 1196,
      "call": "GraphQL\\Executor\\ReferenceExecutor::resolveField()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 264,
      "call": "GraphQL\\Executor\\ReferenceExecutor::executeFields()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php",
      "line": 215,
      "call": "GraphQL\\Executor\\ReferenceExecutor::executeOperation()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/Executor/Executor.php",
      "line": 156,
      "call": "GraphQL\\Executor\\ReferenceExecutor::doExecute()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/GraphQL.php",
      "line": 162,
      "call": "GraphQL\\Executor\\Executor::promiseToExecute()"
    },
    {
      "file": "/home/www/vendor/webonyx/graphql-php/src/GraphQL.php",
      "line": 94,
      "call": "GraphQL\\GraphQL::promiseToExecute()"
    },
    {
      "file": "/home/www/vendor/craftcms/cms/src/services/Gql.php",
      "line": 545,
      "call": "GraphQL\\GraphQL::executeQuery()"
    },
    {
      "file": "/home/www/vendor/craftcms/cms/src/controllers/GraphqlController.php",
      "line": 172,
      "call": "craft\\services\\Gql::executeQuery()"
    },
    {
      "call": "craft\\controllers\\GraphqlController::actionApi()"
    },
    {
      "file": "/home/www/vendor/yiisoft/yii2/base/InlineAction.php",
      "line": 57,
      "function": "call_user_func_array()"
    },
    {
      "file": "/home/www/vendor/yiisoft/yii2/base/Controller.php",
      "line": 181,
      "call": "yii\\base\\InlineAction::runWithParams()"
    },
    {
      "file": "/home/www/vendor/craftcms/cms/src/web/Controller.php",
      "line": 190,
      "call": "yii\\base\\Controller::runAction()"
    },
    {
      "file": "/home/www/vendor/yiisoft/yii2/base/Module.php",
      "line": 534,
      "call": "craft\\web\\Controller::runAction()"
    },
    {
      "file": "/home/www/vendor/craftcms/cms/src/web/Application.php",
      "line": 278,
      "call": "yii\\base\\Module::runAction()"
    },
    {
      "file": "/home/www/vendor/yiisoft/yii2/web/Application.php",
      "line": 104,
      "call": "craft\\web\\Application::runAction()"
    },
    {
      "file": "/home/www/vendor/craftcms/cms/src/web/Application.php",
      "line": 263,
      "call": "yii\\web\\Application::handleRequest()"
    },
    {
      "file": "/home/www/vendor/yiisoft/yii2/base/Application.php",
      "line": 392,
      "call": "craft\\web\\Application::handleRequest()"
    },
    {
      "file": "/home/www/web/index.php",
      "line": 21,
      "call": "yii\\base\\Application::run()"
    }
  ]
}

[joshuabaker]

It seems it is missing the helpers in the head of the file

That’ll serve me right for just pasting from my local. Updated.

Argument 1 passed to craft\\helpers\\UrlHelper::isFullUrl() must be of the type string, array given

This is an interesting one, because it suggests there’s no value for url in the ‘outer’ JSON.

[denisyilmaz]

I double checked with cleared caches. This error is happening to the oembed fields that were throwing the error from #74 before (where there seems to be something mis-saved in the database). I'm not to familiar with migrations in craft but I think we must clear the saved values for the oembed field with the update so your new logic is used on every occurance of the oembed field

--- edit

its fixed in the latest Commit 6c2666d

[denisyilmaz]

this fixed the error i mentioned in the PR!

[joshuabaker]

@denisyilmaz Careful that some aren’t null. I’m realising that this is a recursive issue. See the third row of my comment. In that row url is nested twice, so there will be select scenarios where even the second check fails (or returns null silently). It’s likely we’ll need a while loop to check value isn’t an array with a url value, and then throw after if value is still an array.

Broader changes are going to be needed here that this PR. For instance, why are we storing JSON and then just throwing away all but the url when we normalise? Lots to clean up.

N.B. This PR is to fix #74 alone.

[denisyilmaz]

wouldnt this be enough (Line 126):

// If array with `url` attribute, that’s our url so update the value
while(is_array($value)) {
    $value = ArrayHelper::getValue($value, 'url');
}

[denisyilmaz]

is there a reason thats blocking the merge? @joshuabaker did this work with while for you?

[joshuabaker]

@denisyilmaz Your approach makes sense and I’m not intending to block the merge here (deep in something else). As noted above, my changes are just to address #74 and not the other things I noted.

@reganlawton asked me to address caching, however, the current implementation doesn’t cache anything, instead throwing away the JSON per session. Not sure how to proceed.

[denisyilmaz]

This should take care of caching:

    /**
     * @var null|mixed
     */
    private $_value;

    /**
     * @param mixed $value The raw field value
     * @param ElementInterface|null $element The element the field is associated with, if there is one
     *
     * @return mixed The prepared field value
     */
    public function normalizeValue($value, ElementInterface $element = null)
    {

        if ($this->_value !== null) {
            return $this->_value;
        }
        $this->_value = null;

        // If null, don’t proceed
        if ($value === null) {
            return null;
        }
        
        // If an instance of `OembedModel` and URL is set, return it
        if ($value instanceof OembedModel && $value->url) {
            $this->_value = $value;
        }

        // If JSON object string, decode it and use that as the value
        if (Json::isJsonObject($value)) {
            $value = Json::decode($value); // Returns an array
            $this->_value = $value;
        }

        // If array with `url` attribute, that’s our url so update the value
        while(is_array($value)) {
            $value = ArrayHelper::getValue($value, 'url');
            $this->_value = $value;
        }
        
        
        // If URL string, return an instance of `OembedModel`
        if (is_string($value) && UrlHelper::isFullUrl($value)) {
            $this->_value = new OembedModel($value);
        }
        
        return $this->_value;
    }

[reganlawton]

I moved this PR to use the branch alpha does anyone have an issue with me pull this and changing the change from @denisyilmaz and then getting a version bump?

[reganlawton]

This will make using in composer easier at the very least for testers

[joshuabaker]

I moved this PR to use the branch alpha

Wait, why? The alpha branch isn’t production stable, I take it? We should be fixing this for the latest production-ready release. The changes can merge to alpha too.

[reganlawton]

@joshuabaker Are you about to merge @denisyilmaz changes or did you want me to make it in another branch as I cant merge your code without merging your branch.

[joshuabaker]

@reganlawton No, got for it. Thanks 😊

[joshuabaker]

@reganlawton Where are we at with this one? If there’s more to be done, please can you provide a checklist of tasks.

[joshuabaker]

@reganlawton changed the base branch from master to alpha 7 days ago

I just noticed that you went ahead and merged this with the alpha branch. This is a hotfix for the master branch!

The issue is currently impacting production sites.

[reganlawton]

I'm on this today