Make WordPress Core

Opened 6 years ago

Last modified 6 years ago

#47083 new defect (bug)

Site Info Update Not Adjusting "Last Updated" Value

Reported by: conner_bw's profile conner_bw Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 5.1.1
Component: Networks and Sites Keywords:
Focuses: multisite Cc:

Description

+ Navigate to /wp-admin/network/site-info.php?id=XXX where XXX is a valid Site ID.
+ Checkmark: Spam
+ Click: Save Changes

Expected: "Last Updated" changed to current time.
Actual: "Last Updated" not changed.

---

This is in contrast to /wp-admin/network/sites.php -> Bulk action -> Mark as Spam.
Doing it bulk updates the Last Updated field to current time.

---

In fact, behaviour is broken since 5.1.0

update_blog_details function used to do:

	$details = array_merge($current_details, $details);
	$details['last_updated'] = current_time('mysql', true);

https://github.com/WordPress/wordpress-develop/blob/5.0/src/wp-includes/ms-blogs.php#L297

---

This is a problem because we use "Last Updated" field to purge spam. It gives users X days from the "Last Updated" value to dispute the spam.

Change History (3)

#1 @tmdesigned
6 years ago

I was able to replicate this behavior. The individual site setting change did not update the "last updated" value, while the bulk action did.

The lines of code you called out:

$details = array_merge($current_details, $details);
$details['last_updated'] = current_time('mysql', true);

actually do still exist. Much of that code was just abstracted into the new wp_update_site function (currently ms-site.php, starting line 150). However, while the current datetime is being calculated, it is processed through wp_prepare_site_data, where it is treated as a "default" value against which the form data value is compared.

When editing a single site, the form value of last_updated is submitted along with the other values--it is one of the form fields on that page, after all--so that value for the datetime wins out over the "default".

When bulk editing sites, only the selected bulk attribute is provided (i.e. spam), so the new datetime, which again is treated as a default, is updated.

It looks like previous to 5.1, the datetime was being calculated and added to the payload after the form values were pulled, so the calculated time won out over whatever you entered there.

I do not know if this behavior change is intended. My own opinion of the best behavior would be to compare the form value of the datetime with the existing stored value. If they are the same (meaning the user did not edit the field), then it should be updated with the calculated time. If they are different, it should use whatever they typed in.

#2 @conner_bw
6 years ago

The lines of code you called out:
https://github.com/WordPress/wordpress-develop/blob/5.0/src/wp-includes/ms-blogs.php#L297
actually do still exist.

I disagree. It does not.

The linked code explicitly overrides:

$details['last_updated'] = current_time('mysql', true);

~7 lines later, the code does:

$result = $wpdb->update( $wpdb->blogs, $update_details, array('blog_id' => $blog_id) );

I don't see any ambiguity between those 7 lines of code? In 5.0, last_updated was always set to the current time.

abstracted into the new wp_update_site

Whoever refactored the code changed the behaviour. If it was intentended, then they need to fix bulk update to match.

I don't think it was?

Last edited 6 years ago by conner_bw (previous) (diff)

#3 @tmdesigned
6 years ago

I think you misunderstood me. I did not disagree that there was a code behavior change.

The change was not because those lines were removed, though. It does still calculate the current time using the same exact 2 lines of code (moved elsewhere), but now it only uses it as a default for when a value isn't provided. A value is provided from the form when editing a single site, so the "default" isn't used.

Allowing a user to submit an arbitrary "last updated" value is a little odd no matter how you think about it. Some options:

1) Always update the last_updated value to the current time, ignoring what the user may have typed into the form, thereby reverting to the pre-5.1 behavior. If this is done, I would recommend removing it as a value from the form altogether and not displaying it in an input box, since that implies you can edit it.

2) Change the bulk edit function to not update the last_updated value. This seems like the least favorable solution, since it is in fact now edited.

3) On the single site update form, compare the submitted "last edited" value to the existing (previous) value. If they are the same, meaning the user did not edit the input box, update the time to the present.

The third is more flexible, but has the odd effect that if for some reason I wanted the last updated time to stay exactly the same, I'd have to first change it to something else (or let it update automatically) and then change it back. Perhaps to address this there is a 4th option

4) Add a checkbox field, "Update to current date and time", that defaults to being checked. If checked, the new calculated datetime can be used. If unchecked, whatever is in the form (which starts as the previously saved value) will be used. Uncheck the box automatically if the user changes the value in the input box.

Note: See TracTickets for help on using tickets.