backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

Fixed: `node_cron_last` state is stored as a string. Similar dates are stored as integer. #5849

Closed yorkshire-pudding closed 1 year ago

yorkshire-pudding commented 1 year ago

Description of the bug

While working on https://github.com/backdrop-contrib/bee/issues/153 I noticed that in the state table, while most date based states are stored as an integer, node_cron_last is stored as a string.

Steps To Reproduce

To reproduce the behavior:

  1. Start a new site (though you will see this in any existing site also)
  2. Either using mysql at the command line or in another tool check the values in the state table

Actual behavior

cron_last | i:1667310477; node_cron_last | s:10:"1667311323";

Expected behavior

cron_last | i:1667310477; node_cron_last | i:1667311323;

Additional information

Add any other information that could help, such as:

indigoxela commented 1 year ago

Confirmed (and also confused):

The node_cron_last state is derived from a $node->changed value, which is supposed to be an integer, but is actually a string after loading the entity.

Definition in node.entity.inc:

  /**
   * The node creation timestamp.
   *
   * @var integer
   */
  public $created;

  /**
   * The node modification timestamp.
   *
   * @var integer
   */
  public $changed;

But then...

$node = node_load(1);
debug($node->changed); // <- A string!

In the database it's an integer, when it gets set initially it's an integer (REQUEST_TIME), after node_load, or entity_load it's a string. Not sure if that's as expected/designed, and I'm also not sure if this is helpful here, anyway. :wink:

argiepiano commented 1 year ago

First, question: does the fact that this value is stored as a string cause any issues for you, @yorkshire-pudding? strings and integers are pretty fluid in PHP. Try 2 + '2' and you'll get 4.

And second, for the property changed to be an int, it must be type-hinted as int in the class declaration. Apparently (although I'm not finding any documentation for this), when you use the PDO::FETCH_CLASS to fetch values from a db query to create a class with properties for each column (like DefaultEntityController::load() does), if the property is just defined as public, it gets assigned as a string. Changing public $changed to public int $changed makes that an int in the object. Same problem (non-problem) with promote and other int columns.

yorkshire-pudding commented 1 year ago

Hi @argiepiano - no it doesn't cause any ongoing issues for me; it caused me quite a bit of confusion and red-herring chasing while testing the aforementioned bee state-set and bee stage-get functions. Left as is, it may cause confusion for people using those functions. My main thought was to make it consistent with other similar states. There have been other issues about getting states to be the right type before, so I thought it would make sense.

My PR is pretty simple, but if no-one wants it, I'm not too bothered.

argiepiano commented 1 year ago

Sounds good. I hadn't noticed the PR. LGTM!

bugfolder commented 1 year ago

Code reviewed: LGTM.

Tested:

WFM and RTBC.

quicksketch commented 1 year ago

Thanks folks! I feel like there might be a better way to keep node properties like created and changed as integers all the time, but I'm not too fussed about this simple fix to ensure the config value is an integer. I merged https://github.com/backdrop/backdrop/pull/4250 into 1.x and 1.23.x. Thanks!