Page MenuHomePhorge

Make src/infrastructure/javelin/markup.php phabricator_form PHP 8.1 compliant
ClosedPublic

Authored by Sten on Jun 30 2023, 13:39.

Details

Summary

src/infrastructure/javelin/markup.php phabricator_form() performs a strcasecmp() operation on an optional attribute withoiut checking to see if it exists first. This causes an 'Passing null to parameter #1 ($string1) of type string is deprecated' error.

Because we subsequenly check to see that the value equals /POST/i, all we need to do is check that the value has some 'true' value before doing the strcasecmp(). If it's not true, then it won't be POST!

Fixes T15509

Test Plan

Diff Detail

Repository
rP Phorge
Branch
form (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 615
Build 615: arc lint + arc unit

Event Timeline

Sten requested review of this revision.Jun 30 2023, 13:39

Thanks! You can just leave $http_method without any bracket :)

(Note that if($something) involves a PHP pitfalls that excludes the string 0 - but that is under control here since 0 is not a valid HTTP method, and it's surely not POST.)

src/infrastructure/javelin/markup.php
77

Yes - much nicer. Thanks!

Sten marked an inline comment as done.Jun 30 2023, 14:02

Thaanks!

Tested locally, and logged the values with phlog to verify with my big eyes that everything was under control. I was able to capture both cases:

# phlog($http_method);
# phlog($is_post ? 1 : 0);

'POST' at [markup.php:78]
1 at [markup.php:79]
null at [markup.php:78]
0 at [markup.php:79]

lgtm

This revision is now accepted and ready to land.Jun 30 2023, 14:06

Is there another place not specifying the method for a form? I don’t think that attribute should be optional and instead the fix is to explicitly declare GET or POST or PUT for forms.

In D25319#9336, @speck wrote:

Is there another place not specifying the method for a form? I don’t think that attribute should be optional and instead the fix is to explicitly declare GET or POST or PUT for forms.

phabricator_form() is called in 24 places, and 23 of those specify the method.

So the choices are:

  1. Update the one calling place which isn't specifying the method, but leave phabricator_form() as a fragile function.
  2. Update phabricator_form() to defensively handle the lack of a method attribute, as we have done here.
  3. Update the phabricator_form function signature to make the method a required parameter.

I'm not comfortable in my ability to adequately test the 24 places which call phabricator_form(). Additionally, a couple of these are classes which have the method set to POST by default, but provide setMethod() calls with no checks on what's set, so these would also need updating to ensure only valid methods are set.

Pragmatically, I see sticking with option 2 as the best option.

phabricator_form() is called in 24 places, and 23 of those specify the method.

So the choices are:

  1. Update the one calling place which isn't specifying the method, but leave phabricator_form() as a fragile function.
  2. Update phabricator_form() to defensively handle the lack of a method attribute, as we have done here.
  3. Update the phabricator_form function signature to make the method a required parameter.

I prefer option #1 here and don't see this as leaving phabricator_form as fragile, but having a contract that the method must be specified. I expect that to be the intent given that in 24 invocations all but one explicitly sets the method, which seems like the outlier.

I'm not comfortable in my ability to adequately test the 24 places which call phabricator_form().

I don't follow -- updating one call site only requires testing that one call site and not all 24.

Additionally, a couple of these are classes which have the method set to POST by default, but provide setMethod() calls with no checks on what's set, so these would also need updating to ensure only valid methods are set.

Classes having the method default to POST, but here in phabricator_form the default is GET if it's not specified. This feels confusing to me. I would rather have an assert if the method isn't specified. Btw this was addressed (but never landed) in D25268.

I'm generally not a fan of extra defensive logic in code, with exception to e.g. serialization or backwards compatibility. The result is allowing for more confusing code at the call sites which makes it difficult for maintainability in the long term.