On Thursday, May 26, 2022 at 11:41 AM, Craig Francis <cr...@craigfrancis.co.uk (mailto:cr...@craigfrancis.co.uk)> wrote: > On 26 May 2022, at 15:01, Michael Babker <michael.bab...@gmail.com> wrote: > > On Thu, May 26, 2022 at 7:21 AM Craig Francis <cr...@craigfrancis.co.uk> > > wrote: > > > That said, I would still like to know about the benefits of rejecting > > > NULL for `htmlspecialchars()`, in the context of that Laravel Blade > > > patch; because, if it is beneficial (vs the BC break), they should revert > > > that patch... shouldn't they? > > > > No, they should not. Laravel made an explicit choice to handle null values > > in its escaping function for its templating framework. That is their choice > > to make. They should not be forced to implicitly handle null values because > > the language decides to add implicit type coercion to user defined > > functions (because consistency seems to be so important to some), and they > > should not be forced to reject null values because underlying language > > functions reject them as well. > > > Erm, I'm simply asking - If there is a good reason for throwing an exception > when NULL is passed to `htmlspecialchars()`, then that reason would also > apply to Laravel Blade for it's HTML encoding.
Laravel’s `e()` helper function is more than a wrapper around `htmlspecialchars()` which supports values beyond a string, and has elected to support a null value being passed into that function with explicit handling for it. I’ve seen similar patches land in other packages as well. Whether it is to prevent that package from triggering the deprecation regarding null values itself or the package owners choosing to explicitly handle the null case so users don’t need to deal with it doesn’t really matter, the point is folks have made that decision and it is not problematic in any way IMO. You’re basically saying that a framework choosing to apply the same `htmlspecialchars($string ?? ‘’)` patch that applications are suggested to use (for those who don’t care to differentiate between null and empty string before something reaches an escaping function) is in the wrong by arguing for Laravel to revert that patch. On Thursday, May 26, 2022 at 11:41 AM, Craig Francis <cr...@craigfrancis.co.uk (mailto:cr...@craigfrancis.co.uk)> wrote: > > If a function, by explicit design, can safely work with null values then > > that parameter should be properly declared nullable. > > > `htmlspecialchars()` can and always has safely worked with NULL, the same > with the other 335 parameters I've listed. But when I suggested changing > these parameters to be nullable, it was not well received. The arginfo for the `htmlspecialchars()` function, and if I’m reading correctly (I’m no C developer so I could very well be misinterpreting things) the internal `php_html_entities()` function which `htmlspecialchars()` calls, does not declare null as a supported value for the `$string` parameter. Meaning that depending on whether your code uses strict_types or not, either calling the function with a null value triggers a type error or requires implicit type coercision to convert the value to a type that the function does support. This means to me that the `htmlspecialchars()` function does NOT support null values and that passing null through only works because of the reliance on implicit type coercision. That, to me, is a valid reason for a type error to be raised; the code very explicitly requires a string and is not written to process a null value. A blanket suggestion to make every string parameter which implicitly supports a null value nullable because they previously supported null through type coercion IMO should be shot down. Instead, parameters which are emitting a deprecation notice because they are receiving an unsupported null value should be reviewed, and if that function can by design support null values, those parameters should be updated to reflect nullability. IMO, `htmlspecialchars()` is not a function which should expliciitly support a null value and the present deprecation is correct. On Thursday, May 26, 2022 at 11:41 AM, Craig Francis <cr...@craigfrancis.co.uk (mailto:cr...@craigfrancis.co.uk)> wrote: > On 26 May 2022, at 15:01, Michael Babker <michael.bab...@gmail.com> wrote: > > Yes, that means that there is a lot of work involved for folks between now > > and the release of PHP 9.0 to either refactor code to avoid type errors or > > to get the language to expand the supported types in appropriate functions, > > but IMO that effort from all parties is worth it in the long run to ensure > > language consistency (I really don’t think userland PHP and > > internal/extension PHP should have vastly different behaviors, and type > > coercion support based on where a function is designed is one of those > > holes that needs filled) and to ensure all APIs explicitly declare what > > types of data they support. > > > > I don't think userland and internal functions should have different > behaviours either (my RFC does say "keep user-defined and internal functions > consistent"). > > To achieve that, for those not using `strict_types=1`, I'm simply suggesting > that NULL coercion continues to work for internal functions, and we update > user-defined functions to allow NULL coercion. On the record, I personally disagree with the strict_types declaration’s existence purely because it makes the runtime inconsistent based solely on what file something was called from, plus there are way to bypass a developer’s explicit opt-in to strict_types being enabled which makes it a “mostly strict but there’s still a gotcha” declaration at best. I don’t think this type of deep rooted behavioral difference improves PHP in any way, but that ship has long sailed. With that said though, I disagree with exposing the internal type coercion behavior to userland code. I think the userland behavior as it is today is the correct behavior, even if I look at implicit type coercision as a bug which will cause hard-to-identify production issues for somebody at some point down the road. Let’s also be real here, a not-so-insignificant number of PHP builds are powered by platforms which have minimal if any automated testing and even less static analysis, meaning a static analyzer is not going to be of much help to those builds to begin with because they’re using non-analyzed code as a foundation. Changing the language in a way that makes these already fragile platforms even more suseptable to hidden type-related bugs is a bad idea. Personally speaking, I’ve been bitten by enough type-coercion related bugs in my career (and continue to address new issues arise thanks to a combination of poorly formed legacy data in the database plus open source packages becoming stricter in the types they support) that I would rather advocate for the language moving in a direction that makes type-related issues harder to hit and it to be noisy when it does encounter such issues; the changes in PHP 8.1 IMO are a step in the right direction.
