- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Frist of all, it's Christopher's fault for not submitting a clearer modification in the PR.
Unnecessary parens added for clarity.
Admin
I've seen this paradigm when wannabe rockstar primadonna programmers get shuffled off into a maintenance position.
"Grr! How dare that oik mess with my code? Reject that! You mean he was fixing a bug? How dare he! I'm the only one allowed to fix bugs here! Now lessee ... what's wrong? Oh yes, simple enough, ..." etc.
Admin
TRWTF is a medium-sized column for a thumbnail! ;)
Admin
I’m not good with PHP, but why the “1 == false” bit. Why not just “false”?
Admin
Nobody is good with PHP, PHP is not good with itself, and PHP programmers are not good with their brains. It's an all around bad situation.
Admin
You would think the maintainer in question tested his "fix" at least once with the key scenario (no thumbnail)? Right??...
Admin
I think you might be misreading the null coalescing operator
Making sure there is a value to compare against before checking if it's 1
Admin
Yeah. The parentheses would have been even better in the wrong solution where
1 == 1leaps out at me for some reason even though precedence means it isn't that.And I definitely would have used
($show_image ?? 0) == 1to keep the types as consistent as possible. In fact, I wonder if that was the reason for the rejection.Admin
Personally I woudl sprinkle in some more parenthesis, not everyone has the operator precedencee memorised; who knows, some other poor soul might have to replace me one day, so I at least try to ease their suffering a little bit.
Admin
To clarify (since I didn't include any code snippets other than the offending one), the class is being set on the div that contains the article summary. If the article has a thumbnail, the summary div gets reduced to col-8 on viewport widths above 768px to make space for the thumbnail next to it, otherwise the summary div uses the whole width (in theory at least). So on anything bigger than a mobile phone, the thumbnail essentially takes up 33% of the column width leaving the rest for the summary
Admin
This - verbosity as long as the formatting is also clean and consistent is never really a sin. 20 years or more ago when being able to fit a full but still reasonably sized function/method/procedure on the screen was sometimes an issue there was an argument for saving a line break or two and shorting a line by 3 or 5 characters; but in 2020 syntax sugars really don't enhance readability. What they add is human-lay ambiguity because I might not remember certain details about precedence or scope, especially if its not a language I use daily.
I am not suggesting we should go in the direction of COBOL or anything wild like that; but generally engaging in code golf is a bad idea.
Admin
If only a very simple rule was followed: No changes to the production code without a corresponding unit test(s). Then if the maintainer changed the code and broke it, the tests would fail; and it would be obvious who was responsible.
Admin
Oh you sweet summer child.
Admin
Writing an automated unit test for "if X, then page renders like Y" is a mite more complex, though. (I inherited a wrapper around Selenium that at least covers simple things like "page contains Y". And in today's example, you could move the relevant code inside a function and then write unit tests for the output of said function.)
Admin
The description isn't really clear on this part. It could have been as simple as isset($show_image) if 0 or false should show an image, or if they shouldn't (as is more likely) $show_image ?? false would have worked just fine.
All these comments here from people who think the == 1 is necessary simply don't understand type coercion.
Admin
I'm really kind of tired of the hate you direct at PHP and the people who use it.
Admin
TRWTF is the lack of parans, the code is ambiguous af. The maintainer obviously didn't understand what problem was being solved, Perhaps they aren't familiar with the (relatively) modern NULL coalescing operator. ($show_image ?? false) ? ' col-md-8' : ''; would have been much clearer and cleaner.
Admin
A counterpoint to that, especially for nontrivial and/or critical code, is that the flow there should always be: receive submitted patch, figure out what it's fixing, do an independent implementation of a fix (which may be the same as what's submitted), integrate and test. There can be a big difference between "I made this change and now it seems to work OK" and "this change resolves the issue taking into account issues X, Y, and Z that also need to be handled with".
Admin
There's something a bit more worrying going on.
"There was just one problem: if a post was published without a thumbnail, attempts to view that post failed with an error ... The problem was, if there was no thumbnail, it wasn't set. And that caused the error."
So this CMS is spamming "Undefined variable" warnings in production (specifically, shoving them into the class attribute)? Because in any live environment with a responsible adult in charge, the warning would have gone to the logs and the code would have continued, using NULL for the missing value, which would test as false, and no-one would have seen a problem.
And historically, back when the line was written, that is probably what happened. Undefined variables were considered mere "poor style, but PHP can deal with it" issues, and generated a Notice-level message. ("In most cases, accessing an undefined variable is a severe programming error. The current low classification is a legacy from the Dark Ages of PHP, where features like register_globals made conditionally defined variables more typical, and code quality standards were lower.") Suppress Notices and everything would appear to run fine. But then someone upgraded PHP to version 8, which upgraded those Notices to Warnings and suddenly they were getting past the suppression.
Admin
Anybody who thinks that the
== 1can be optimised away completely without risk does not understand legacy code.Admin
It's not an optimization, and like I said, it depends on how exactly the values are used, but if the values are 1, 0, and undefined, the == 1 does nothing.
Admin
This doesn't make sense.
I expect 99.999% of all home-grown CMS-systems to do little more than replacing something like "{{ ... }}" by whatever PHP evaluates "..." to be (hopefully after some escaping). The normal PHP-rules apply in that case, so an unset $show_image would simply evaluate the expression $show_image == 1 to false (without raising an error).
If it actually did result in an error (one that caused failure to view the post, just by referring to an unset variable), the home-grown CMS must have rewritten parts of the ZEND-core, changing PHP's behavior. Hard to imagine that's the case without such a "detail" being mentioned.
I'm sure there are real problems in the code, but the problem mentioned in this article appears to be an oversimplification.
Admin
Null coalescing may be new but the warning suppression operator was in PHP since likely ever. So the quickfix could be
@$show_image == 1 ? ' col-md-8' : ''.