Laminas\Json dependency removal

[gsteel]

Q	A
Documentation	yes
Bugfix	yes
BC Break	yes
New Feature	no
RFC	no
QA	yes

Description

As per TSC #68, this pull removes the dev dependency on Laminas\Json.

• Removes Laminas\Json from deps
• Replaces encoding using built-in json_encode with JSON_THROW_ON_ERROR. Mostly, Json exceptions are wrapped to throw a Zend\View\Exception, which I guess is a BC break?
• c6537c1 Fixes missing fluent return
• The Json view helper no longer does the Json\Expr stuff, so if the option to turn this thing on is present, a deprecation warning is issued - perhaps this is not the best way to deal with that scenario.
• Fixes silent fail on Json::encode Error #33

[Ocramius]

This should either go to 2.13.x (not yet there) or 3.0.0, but not 2.12.x (current stable - can't change dependencies in patches)

[gsteel]

OK @Ocramius - I'll rebase later on. Would 3.x be best considering the impact on the Json view helper? Perhaps a deprecation warning should be issued in 2.13.x if usage of the Json\Expr finder is detected?

[Ocramius]

@gsteel I'm mostly waiting for github actions to resume operations: currently, all builds seem to be hanging for some reason.

I'm wondering if we should open 2.13.x, but perhaps @boesing has a better idea of the plans for this package 🤔

[gsteel]

Oh yeah… literally everything is hanging in the balance 😬 https://www.githubstatus.com/incidents/xn0sd2x4nd7f

[boesing]

I'm wondering if we should open 2.13.x, but perhaps @boesing has a better idea of the plans for this package 🤔

TBH, I dont have any plans on this package. 😅
But changing laminas-json exceptions to native JsonException might be a problem and introduces BC. 🤔

[Ocramius]

Good catch on the exception type - very hidden BC boundary. Probably indeed better to re-target 3.0.0

[gsteel]

I should probably rebase later on once 2.12/2.13 is in the works yes?

Also, it's effectively a feature removal if anyone was using the Json Expression finder feature via the view helper and Laminas\Json package, so perhaps a new pull targeting 2 to announce future removal??

[Ocramius]

I should probably rebase later on once 2.12/2.13 is in the works yes?

Yeah, but hold off yer rebases, since we first need to get 2.13.0 green (see #71 if you have time, specifically current CI failures around markdown)

[boesing]

Probably, providing some kind of forward compatibility layer might be a good idea.
Having a way to prepare for changes is always a good one.

[gsteel]

Would it be weird to introduce a Json View Helper into the to-be-deprecated Laminas\Json package so that anyone requiring that functionality could depend on that? Then the current view helper could be replaced with what's effectively in this pull.

[boesing]

Actually, it is not really deprecated but security-only (feature-complete). However, I think in order to manage forward compatibility, it might be possible.
@weierophinney thoughts?

[Ocramius]

Well, needing laminas/laminas-json is weird anyway, since JSON_THROW_ON_ERROR effectively replaces all of it :|

[gsteel]

Also, there's #33 which makes sense after looking at Laminas\Json.

[weierophinney]

@Ocramius —

Well, needing laminas/laminas-json is weird anyway, since JSON_THROW_ON_ERROR effectively replaces all of it :|

That's not entirely true in the case of this particular component's usage of it. One of the options you can pass as the second argument to the json() helper is 'enableJsonExprFinder'. When true, it passes that flag on to Laminas\Json\Json::encode, which then encodes any Laminas\Json\Expr instances as native JS expressions; the primary use case is for encoding closures that will then be used within <script> tags, so they can be executed by the browser. ext-json has no such concept. Which is what leads to the questions here of whether or not we can we remove laminas-json safely here, or if it would be a BC break.

I think in this case, we have two options:

• Identify when the "enableJsonExprFinder" option has been provided, trigger an E_USER_DEPRECATED error, and use laminas-json to encode the content. We then remove the functionality in v3. (Basically, drawing a sand in the line and saying JS expression support goes away entirely.)
• Move the functionality to a new package, but name the helper differently (e.g., jsonWithExpr()). This package would detect requests to "enableJsonExprFinder", and, when detected, proxy to the new helper. If we go this route, we could still deprecate that usage, and encourage users to use the new helper when they want that support. v3 removes the support for "enableJsonExprFinder", and drops the dependency on the package providing the new jsonWithExpr() helper.
  • That functionality could be pushed into laminas-json instead of a new package; this is one of the few reasons I'd consider us doing another release of that package, TBH.

Honestly, I could go either way. I think there are other ways to accommodate expression support (e.g., adding the property with an expression within the body of the <script> tag), and, since it's not something any other language can understand (you cannot send it over the wire and evaluate it in JS), dropping it entirely in v3 would be fine.

[Ocramius]

Something went awfully wrong with the commit range here

[gsteel]

Something went awfully wrong with the commit range here

Yeah… I think I might just start fresh from 3.x and close this one. The biggest problem is MVC/i18n with everything in 3.x and the dep on root. I have no idea how to get deps installed on 3 because the mvc stuff wants view ^2.

Do you have any pointers?

Also, would it OK to create a new pull removing flash, console, json and other deprecations all in one go?

[gsteel]

This has been re-created in #125