ezplatform-graphql icon indicating copy to clipboard operation
ezplatform-graphql copied to clipboard

IBX-7603: Added missing subfield for ezurl

Open mateuszdebinski opened this issue 1 year ago • 9 comments

Jira: https://issues.ibexa.co/browse/IBX-7603

If the ibexa.graphql.schema.should.extend.ezurl parameter is false (default), the graphQL query will look like before, i.e.:

{
  content {
    urlCT(contentId:68) {
      urlField
    }
  }
}

but if we set this parameter to true, the graphQL query will look like this:

{
  content {
    urlCT(contentId:68) {
      urlField {
        link,
        text
      }
    }
  }
}

mateuszdebinski avatar Feb 05 '24 15:02 mateuszdebinski

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Feb 05 '24 15:02 sonarqubecloud[bot]

@mateuszdebinski could you please provide example query that will no longer work? The description is close to non-existent, it's hard to tackle the exact problem judging only by the schema adjustment.

konradoboza avatar Mar 07 '24 09:03 konradoboza

@mateuszdebinski could you please provide example query that will no longer work? The description is close to non-existent, it's hard to tackle the exact problem judging only by the schema adjustment.

@konradoboza I added what it looks like now and what it will look like after the change, The current query will not work without specifying what fields you want to extract from the query

mateuszdebinski avatar Mar 07 '24 09:03 mateuszdebinski

creating another dedicated GraphQL endpoint though. We can't really do that, the graphql endpoint is kind of "unique".

But BC is a problem here. If a project uses an ezurl field over GraphQL, their existing query will break, period. I don't think it's acceptable.

GraphQL doesn't have anything for changing a primitive type to a custom one, afaik. The only way I can think of is some kind of feature flag. We add a custom mapper for that field type, and in the mapper, we either String or UrlFieldValue depending on the flag.

As it doesn't seem very likely that this is widely used, and in order to maximize the adoption of the new schema, we could disable the BC flag by default, and document that it must be enabled if such a field type is used. Of course, updating the client code is a better option.

bdunogier avatar Mar 07 '24 10:03 bdunogier

@bdunogier I don't know much about GraphQL. Is it possible to keep here BC? Meaning when old query is passed, returning old result? We need to have a solution here.

alongosz avatar Mar 07 '24 12:03 alongosz

Unfortunately, no, @alongosz, no BC features for changing a primitive type to an object. Hence my suggestion above for a feature flag.

bdunogier avatar Mar 08 '24 08:03 bdunogier

Let's complete this one, it's not gonna fix itself :)

The way to fix it is correct, but the BC break isn't. We can change the schema that way in v5.0, but not in 4.6. It is likely that this ezurl is used with GraphQL by existing apps.

We must add a feature flag / configuration directive that enables the new format, and ship it as disabled on 4.x. On 5.0, we can either ship the feature flag and enable it by default, or just change the schema. In any case, we should add a @deprecated flag to the current version of the field.

Is that okay for everybody ?

bdunogier avatar May 29 '24 09:05 bdunogier

As we do not recommend using a lower PHP version than 8.1 and ultimately 7.4, I changed the dependency in composer for PHP to at least 7.4, I don't know if I can do that, so please let me know whether to restore support for 7.3

mateuszdebinski avatar Jul 15 '24 13:07 mateuszdebinski

Sorry, but we need to keep compatibility with PHP 7.3. Bumping minimum required version of package is a BC break.

adamwojs avatar Jul 16 '24 08:07 adamwojs