Should respect jsonapi.base_path container parameter override

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.

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.

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?

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.

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... 🐛 InvalidArgumentException: Expected a scalar value as a 2nd argument to "Symfony\Component\HttpFoundation\InputBag::get()", "array" given. Fixed

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

Applied the style fixes at lease

Greate work <3

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