Problem/Motivation

When JSON:API Extras module is enabled then it looks like it takes over the control on the JSONAPI base path setting. So even if there was a parameter override set for the jsonapi.base_path container parameter (according to the docs), it rolls it back to /jsonapi.

Background story: we have enabled JSONAPI Extras for the field enhancer feature, but we did not expect that it starts overriding existing "hardenings".

Steps to reproduce

* Set up a jsonapi.base_path: /foo parameter override in service.yml
* Enable the JSONAPI Extras module
* The JSONAPI base path rolls back to /jsonapi instead of staying on /foo.

Proposed resolution

Do not override the JSONAPI base path when a path override already exits. Also disable the related configuration option.

Remaining tasks

User interface changes

API changes

Data model changes

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mxr576 created an issue. See original summary.

mxr576’s picture

Status: Active » Needs review

Okay, so tests are passing on D9. My gut says they are not broken on D10 due to the changes that I have made... so needs review.

bbrala’s picture

Status: Needs review » Needs work

Hmm, i wonder, another approach to fixing this might be jsonapi extra's not writing the the parameter if the setting is not filled. But it seems the current form does not allow that. Therefor looking at some setting or parameter to see if we need to do work as extra's would be fine.

I think that this fix in its current is a little opague since it just magically stops overwriting. I would probably opt for an explicit setting to disable the path override in the settings form? Although UX wise that is a little weird. But in regards to migration of old settings is probably the easiest implementation path.

mxr576’s picture

I would probably opt for an explicit setting to disable the path override in the settings form?

How that would work/solve the problem in a scenario when jsonapi.base_path parameter was already defined in services.yml _before_ JSONAPI Extras module was enabled? Should the default value of "disable the path override" setting would be TRUE? Would not that be even more confusing? :)

since it just magically stops overwriting.

If I understand your comment right then you are concerned about the upgrade path for existing projects where JSONAPI Extras should not have overridden the path that was defined in jsonapi.base_path parameter, but it did...

Since this problem could be only solved with more magic... (like a BC config setting that tries to guess whether this was true or not), I'd say since this is a bug fix, the release notes should explain developers that if they experience any issues after the update look for a potential base path configuration mismatch between JSONAPI Extra's settings and services.yml.

Would you accept that or you would rather see a BC setting instead?

bbrala’s picture

How that would work/solve the problem in a scenario when jsonapi.base_path parameter was already defined in services.yml _before_ JSONAPI Extras module was enabled? Should the default value of "disable the path override" setting would be TRUE? Would not that be even more confusing? :)

This is a valid point. To be honest an opt-in approach to the path override does make a lot of sense really. I understand from an initial implementation standpoint that this module started with just overriding and using the default. I do understand this might make implementation a little harder since it would need to also add a update hook to set the setting right.

This would also bring the path override in line with the resourceoverrides which need to be explicitly enabled.

If I understand your comment right then you are concerned about the upgrade path for existing projects where JSONAPI Extras should not have overridden the path that was defined in jsonapi.base_path parameter, but it did...

Since this problem could be only solved with more magic... (like a BC config setting that tries to guess whether this was true or not), I'd say since this is a bug fix, the release notes should explain developers that if they experience any issues after the update look for a potential base path configuration mismatch between JSONAPI Extra's settings and services.yml.

Would you accept that or you would rather see a BC setting instead?

I do think the end goal should be to enable path override explicitly but this can wait for a new major. But the more i think of it i think the current implementation is fine for now. I do agree the release notes would need a proper explaination should things change. But i'm not too worries there will be many unforseen concequences for people after upgrading.

So for now, lets just get this there.

The failures are jsonapi_defaults being not so nice it seems. I've not seen issues running this on a d10 install.

mxr576’s picture

Okay, so if I understand it well the change as-is is acceptable, right? It just requires a detailed release note about the "unexpected" outcomes.

I can ask one of my colleagues to RTBC this because we have been using this patch on some projects already for a while.

The failures are jsonapi_defaults being not so nice it seems. I've not seen issues running this on a d10 install.

This patch fixes that unrelated failure based on my testing: https://www.drupal.org/project/jsonapi_extras/issues/3349731#comment-150...

mxr576’s picture

Status: Needs work » Needs review
Agnes_Varro’s picture

Status: Needs review » Reviewed & tested by the community
bbrala’s picture

Status: Reviewed & tested by the community » Needs work

There are some style issues and we kinda need to use dependency injection :)

bbrala’s picture

Applied the style fixes at lease

mxr576’s picture

Status: Needs work » Needs review

  • bbrala committed 36c18624 on 8.x-3.x authored by mxr576
    Issue #3351101 by mxr576, bbrala, Agnes_Varro: Should respect jsonapi....
bbrala’s picture

Status: Needs review » Fixed

Greate work <3

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.