nystudio107 / craft-seomatic

SEOmatic facilitates modern SEO best practices & implementation for Craft CMS 3. It is a turnkey SEO system that is comprehensive, powerful, and flexible.
https://nystudio107.com/plugins/seomatic
Other
165 stars 69 forks source link

Empty values in JSON should be null not [] #1455

Closed MangoMarcus closed 5 months ago

MangoMarcus commented 5 months ago

Describe the bug

I'm using graphql in a headless nextjs app to query metaTagContainer and metaSiteVarsContainer which each contain fields like this:

  "keywords": [],
  "description": [],
  "referrer": { "content": "no-referrer-when-downgrade", "name": "referrer" },
  "robots": { "content": "none", "name": "robots" },

So it's an object if it's set, and an empty array otherwise. I'm guessing this is because in PHP they're associative arrays, in JSON though (and JS) associate arrays are objects.

Empty arrays are also truthy in JS, unlike PHP where they're falsey.

This means you have to jump though some hoops if for example you want to check that the field is set and use a fallback

To reproduce

Steps to reproduce the behaviour:

  1. Query with GQL using a gql client:
    query GetSeo {
    seomatic(asArray: true) {
      metaTitleContainer
      metaTagContainer
      metaLinkContainer
      metaScriptContainer
      metaJsonLdContainer
      metaSiteVarsContainer
      metaSiteVarsContainer
    }
    }
  2. Parse the JSON: const tagContainer = JSON.parse(seomatic.metaTagContainer)
  3. Log the json

Expected behaviour

I'd expect to be able to do something like

const title = tagContainer.title ? tagContainer.title.value : 'Some fallback';

Instead you have to do

const title = Array.isArray(tagContainer.title) ? 'Some fallback' : tagContainer.title.value;

Or, in the case of the NextJS generateMetadata export, you have to do something like this

export async function generateMetaData() {

  const tagContainer = ... // ... query with gql

  return {
    // ...
    opengraph: {
      title: Array.isArray(tagContainer['og:title']) ? undefined : tagContainer['og:title'].content
    }
  };  
}

Screenshots

If you want to support Typescript, you also need to set up types like this

Screenshot 2024-04-11 at 12 01 49

Versions

khalwat commented 5 months ago

So essentially, you're wanting SEOmatic to send empty empty objects {} vis JSON as opposed to an empty array [] as it is doing now?

khalwat commented 5 months ago

So I had a look at this, and it's using Yii's Json::encode() (which then uses PHP's json_encode()) to encode the various objects as arrays.

I can't just use JSON_FORCE_OBJECT to ensure that all arrays are associative, because that will result in actual non-associative arrays being encoded with keys.

Also the way things work internally, what's actually being returned are a given tag object's attributes, so to be internally consistent, having it return an empty array here is necessary, as much of the internal system relies on this.

However what I can likely do is apply a filter in the context of a returned JSON array of data, such that empty arrays are converted into null at that stage.

khalwat commented 5 months ago

So... I actually wrote the code to do this:

    public static function emptyArraysToNullRecursive($array): array
    {
        $func = static function($item) use (&$func) {
            if (is_array($item)) {
                if (!empty($item)) {
                    return array_map($func, $item);
                }

                return null;
            }

            return $item;
        };

        return array_map($func, $array);
    }

...but given that this is a breaking change (some people certainly have their frontend depending on the current behavior), I think it'd be a better idea for you to just implement the equivalent of that in JavaScript.

Write a map or other function to iterate through the returned JSON result, and swap in null for any empty arrays, and then you can use your preferred syntax, and cleaner TypeScript types as well.

If I were doing it from scratch, I'd likely have it returning null or not returning a key at all... but given that it's a breaking API change, I think it makes sense to adjust it downstream.