aerni / statamic-advanced-seo

Comprehensive SEO addon for Statamic with flexibility in mind
https://statamic.com/addons/aerni/advanced-seo
11 stars 6 forks source link

Version 2 #45

Closed aerni closed 1 year ago

aerni commented 1 year ago

This PR includes many new features, improvements, changes, and fixes.

What's new

What's improved

What's fixed

What's breaking

Narretz commented 1 year ago

Hi @aerni I haven't had time to test out the latest gql changes. Just a quick question, are the yaml files compatible between version 1 and version 2?

aerni commented 1 year ago

Yes. There are no major changes to the data structure. The only breaking change I can think of is if you used the {{ seo }} tag in Antlers, as we are now working with actual variables.

Narretz commented 1 year ago

Hi @aerni finally found time to test v2.

Unfortunately I immediately get an error:

Call to a member function fields() on null

Aerni \ AdvancedSeo \ Subscribers \ OnPageSeoBlueprintSubscriber : 66 extendBlueprint

https://github.com/aerni/statamic-advanced-seo/blob/c63120bd189e0731a8b810f79bc221a3fcd46990/src/Subscribers/OnPageSeoBlueprintSubscriber.php#L66

I simply replaced the previous version of the addon with the feature/version-2 branch and cleared the cache. I still have some seo data in a few entities, but the error to me suggests that the advanced-seo::main fieldset isn't registered even though the files exist.

aerni commented 1 year ago

Hmm, that's odd. Like you said, the fieldset file exists. When do you get that error?

Narretz commented 1 year ago

When I try to open an entity, i.e a page that should get SEO fields.

Could it be that it's because I required advanced-seo via git like this:

    "require": {
        "aerni/advanced-seo": "dev-feature/version-2"
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/aerni/statamic-advanced-seo"
        }
    ]

I can see that when it tries to load the fieldsets from the repository, only my app is shown, not advanced-seo. That suggests the addon isn't registered correctly somehow.

aerni commented 1 year ago

I have no idea. What version of Statamic are you on? The fieldset feature has only been added in v3.3.57.

Narretz commented 1 year ago

Thank you, the version requirement was the cause. It didn't cross my mind that a newer version of Statamic could be required, or that such a mundane feature was only just released last year.

aerni commented 1 year ago

I'll make sure to require the latest version. Also, there are quite a few changes that I didn't yet merge into this PR. Just as a side note.

Narretz commented 1 year ago

Is there also a minium Laravel requirement?

I'm just dumping my findings here in this thread, okay?

  1. When I request graphql like:
query Page {
 entries(collection: "pages") {
    data {
      title
        seo {
    computed {
          twitter_handle
        }
        raw {
          twitter_summary_image {
            id
          }
        }
      }
    }
  }
}

I get this error:

"Statamic\\GraphQL\\Types\\AssetInterface::resolveType(): Argument #1 ($asset) must be of type Statamic\\Contracts\\Assets\\Asset, Statamic\\Fields\\Value given, called in ...\\vendor\\rebing\\graphql-laravel\\src\\Support\\InterfaceType.php on line 23"

      "path": [
        "entries",
        "data",
        0,
        "seo",
        "raw",
        "twitter_summary_image"
      ],

The image is defined in the global SEO site settings.

  1. Speaking of the GraphQL query, I'm confused why some fields are under raw and others are under computed. Isn't twitter_summary_image also kind of computed because it can be set globally and per entity? And why is twitter_image under computed then? Especially since twitter_image seems to be the "raw" asset.
Narretz commented 1 year ago
  1. When you disable all analytics providers in the config, the seoDefaults.site.analyticsDefaults object has no fields:

This is what it looks like in the introspection query:

        {
          "kind": "OBJECT",
          "name": "analyticsDefaults",
          "description": null,
          "fields": [],
          "inputFields": null,
          "interfaces": [],
          "enumValues": null,
          "possibleTypes": null
        },

This throws when the schema is validated (tested with graphql 14.x via gridsome which does schema validation):

GraphQLError: Type analyticsDefaults must define one or more fields

I think it would be simpler if the seo provider fields were always available, and just return null if they are disabled via config.

aerni commented 1 year ago

Thanks for your valuable input.

Laravel Requirement Right now the requirement is "laravel/framework": "^8.77 || ^9.0". But I will likely require Laravel 10 and Statamic 4.0 with the release of v2.

1. Error I fixed the error you mentioned above. I'm actually doing some related refactoring in this branch, which already included this fix.

2. Computed vs Raw As for your question about the computed and raw fields. The raw fields output exactly what you see in the SEO tab of your entries. The computed fields are additional fields that don't exist on the entry. The twitter_image, for instance, will output the generated social images if the social images generator is turned on. If not, it will dynamically output either the twitter_summary_image or twitter_summary_large_image depending on which twitter_card type you selected.

3. Disabled fields I came to the same conclusion and already reworked this behavior in this branch.

Narretz commented 1 year ago

Hi @aerni thanks for the response. A few follow up questions:

  1. does the features-and-conditions branch contain all the changes from this branch? Because the changes are generally stable, I'd like to the fixes / new features in production as soon as possible
  2. Will licenses bought right now be valid for v2?
  3. Do you have a rough time for the v2 release in mind?
aerni commented 1 year ago

I already merge the changes from the features-and-conditions branch into this one. Licenses will still be valid, yes. I don't have a specific release in mind. There is still some polishing to do. I'm trying to get it out of the door in the next two months.

aerni commented 1 year ago

I will also add support for Laravel 10 and drop support for all older versions. And it will require PHP 8.1 as per Laravel requirements.

aerni commented 1 year ago

@Narretz Concerning your confusion about computed and raw fields. Would it help if each computed field included a description?

Bildschirmfoto 2023-02-07 um 17 45 29