• (nodebb)

    Frist of all, it's Christopher's fault for not submitting a clearer modification in the PR.

    <div class="col-12{{ (isset($show_image) && $show_image == 1) ? ' col-md-8' : '' }}">
    

    Unnecessary parens added for clarity.

  • Prime Mover (unregistered)

    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.

  • Yikes (unregistered)

    TRWTF is a medium-sized column for a thumbnail! ;)

  • EJ (unregistered)

    I’m not good with PHP, but why the “1 == false” bit. Why not just “false”?

  • (nodebb) in reply to EJ

    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.

  • (nodebb)

    You would think the maintainer in question tested his "fix" at least once with the key scenario (no thumbnail)? Right??...

  • Jaloopa (unregistered) in reply to EJ

    I think you might be misreading the null coalescing operator

    ($show_image ?? false) == 1
    

    Making sure there is a value to compare against before checking if it's 1

  • (nodebb) in reply to Jaloopa

    Yeah. The parentheses would have been even better in the wrong solution where 1 == 1 leaps out at me for some reason even though precedence means it isn't that.

    And I definitely would have used ($show_image ?? 0) == 1 to keep the types as consistent as possible. In fact, I wonder if that was the reason for the rejection.

  • dereferenced null pointer (unregistered)

    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.

  • Christopher (unregistered) in reply to Yikes

    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

  • Hal (unregistered) in reply to dereferenced null pointer

    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.

  • DrPepper (unregistered)

    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.

  • (nodebb) in reply to DrPepper

    Oh you sweet summer child.

  • (nodebb) in reply to DrPepper

    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.)

  • tbo (unregistered) in reply to EJ

    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.

  • tbo (unregistered) in reply to Mr. TA

    I'm really kind of tired of the hate you direct at PHP and the people who use it.

  • Not Ian (unregistered)

    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.

  • (nodebb) in reply to Prime Mover

    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".

  • Prime Mover (unregistered) in reply to zomgwtf
    Comment held for moderation.
  • (nodebb)

    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.

  • I'm not a robot (unregistered) in reply to Watson
    Comment held for moderation.
  • legacy code warrior (unregistered) in reply to tbo

    All these comments here from people who think the == 1 is necessary simply don't understand type coercion.

    Anybody who thinks that the == 1 can be optimised away completely without risk does not understand legacy code.

  • Best Of 2022 (unregistered)
    Comment held for moderation.
  • tbo (unregistered) in reply to legacy code warrior

    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.

  • Guest (unregistered)

    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.

Leave a comment on “Show Thumbnails?”

Log In or post as a guest

Replying to comment #:

« Return to Article