WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#10619 closed defect (bug) (fixed)

Importing WXR breaks serialized postmeta value

Reported by: JonathanRogers Owned by: westi
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.8.4
Component: Import Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

Because custom field values are passed through the poorly named maybe_serialize() when importing, any values that already contain serialized strings are serialized again, making them useless. Although it would make more sense to me to eliminate the bizarre behavior of maybe_serialize() that may break something else, so I fixed this problem by turning off maybe_serialize() for WXR import.

Attachments (3)

wordpress_import_meta_serialized.patch (1.5 KB) - added by JonathanRogers 5 years ago.
Disables maybe_serialize() when importing custom field values from WXR
unserialize.import.diff (492 bytes) - added by znarfor 4 years ago.
Alternative patch
test-postmeta.wxr (3.0 KB) - added by johncoswell 4 years ago.
Test WXR file with serialized postmeta value

Download all attachments as: .zip

Change History (14)

JonathanRogers5 years ago

Disables maybe_serialize() when importing custom field values from WXR

comment:1 JonathanRogers5 years ago

  • Version set to 2.8.4

comment:2 follow-up: westi5 years ago

  • Milestone changed from Unassigned to 2.9

Not sure this is the best solution here.

May be better to unserialise and pass to add_meta as that is what it expects.

comment:3 in reply to: ↑ 2 JonathanRogers5 years ago

Replying to westi:

Not sure this is the best solution here.

May be better to unserialise and pass to add_meta as that is what it expects.

You mean it may be better to unserialize every custom field value when importing? Is it correct to unserialize values that aren't serialized?

I don't think my solution is the best either. I think that either add_post_meta() shouldn't pass the value through maybe_serialize() or maybe_serialize() shouldn't serialize values that are already serialized. What's the value of serializing something twice? However, I didn't do that because the comments on maybe_serialize() implied that there was some reason for its bizarre and extremely surprising behavior.

comment:4 johncoswell4 years ago

I just got bitten by this bug in a WPMU->WPMU import, so here's my two cents. Theoretically, you should be able to run already-serialized values through maybe_unserialize(), because if it's not determined to be serialized data, it simply returns the original data. I'm fine with unserializing the data before passing into add_post_meta(), since if you're working with serializable data structures in the first place, you'd expect to be feeding those same structures back into add_post_meta().

comment:5 beaulebens4 years ago

  • Cc beau@… added

I have to agree with johncoswell here, I think if something is passed to maybe_serialize which is already serialized, then the answer should be "don't serialize (again)" and the already-serialized value should be sent back.

znarfor4 years ago

Alternative patch

comment:6 znarfor4 years ago

  • Cc znarf@… added
  • Keywords has-patch added

Actually, I think westi was right, we just need to unserialize the data before importing it. My patch above simply does this.

comment:7 westi4 years ago

  • Keywords needs-unit-tests added
  • Owner set to westi
  • Status changed from new to accepted

I think znafor's patch is the best solution here.

On export we put the raw data from the db into the xml file.

Normally when getting the post meta we would have called maybe_unserialize on the value before returning - therefore we should replicate that here before passing to add post meta so that we have the same consistent behaviour.

Could someone generate a small WXR file with some post meta in to test this with and report back on sucess/failure

johncoswell4 years ago

Test WXR file with serialized postmeta value

comment:8 follow-up: johncoswell4 years ago

I made that change to a local copy of WP 2.8.4 and it worked as expected. The meta data was properly stored in the database as a serialized string.

comment:9 westi4 years ago

  • Milestone changed from 2.9 to 3.0

We also have the issue outlined in #9633 we will look at both of these early in 3.0

comment:10 in reply to: ↑ 8 westi4 years ago

Replying to johncoswell:

I made that change to a local copy of WP 2.8.4 and it worked as expected. The meta data was properly stored in the database as a serialized string.

Thank you for the sample file.

I have included it as test data into the wordpress-tests repository and started writing some unit tests against it for this issue.

comment:11 westi4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [12495]) Use maybe_unserialized on all post_meta values on import to ensure we don't end up with double serialized data in the database. Fixes #10619 props znarfor.

Note: See TracTickets for help on using tickets.