Differential D25319
Make src/infrastructure/javelin/markup.php phabricator_form PHP 8.1 compliant Authored by Sten on Jun 30 2023, 13:39.
Details 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 View a diff. Eg: https://my.phorge.site/D1234
Diff Detail
Event TimelineComment Actions 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.)
Comment Actions 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]
Comment Actions 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. Comment Actions phabricator_form() is called in 24 places, and 23 of those specify the method. So the choices are:
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. Comment Actions
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 don't follow -- updating one call site only requires testing that one call site and not all 24.
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. | ||||||||||
