Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pull request to fix issue with semicolon in serialized data #12

Closed
wants to merge 1 commit into from
Closed

Pull request to fix issue with semicolon in serialized data #12

wants to merge 1 commit into from

Conversation

kevindeleon
Copy link
Contributor

As reported in an issue, serialized data was getting replaced and not followed up by the appropriate semicolon. This should take care of that. The issue in question:

#9

Please test and commit when ready.

As reported in an issue, serialized data was getting replaced and not followed up by the appropriate semicolon. This should take care of that.
@veloper
Copy link
Owner

veloper commented Jun 5, 2013

Hey, just wanted to say thank for doing this.

Do you know of any plugins that added serialized options that this script previously messed up on? I'd like to do some kind of testing for this fix is possible.

@kevindeleon
Copy link
Contributor Author

Hey man...no problem.

I do not know...maybe the guy that initiated the issue can tell us which plugins...I will try to contact him.

I honestly just did some testing with some fake data and also did some testing in a regex tester I use (Patterns)...and I didn't see any issues, but I also never really saw them beforehand (guessI never used the plugins in question). I basically just found the problem in the Regex and fixed that. Regex was grabbing all the way to the semicolon, but not replacing the semicolon.

@kevindeleon
Copy link
Contributor Author

@veloper On second thought...after looking at the original requester's serialized example...it looks like maybe "dashboard_incoming_links" was the culprit in his example. I believe that is core is it not?

@veloper
Copy link
Owner

veloper commented Jun 11, 2013

I'll review this in more detail over the coming weekend.

@kevindeleon
Copy link
Contributor Author

@veloper That's cool. According to the comment over here #9 (comment) he is verifying that it was, in fact, just wp-core that was needed to cause any problems with the serialized data. That verifies my previous comment that his test data was using info from "dashboard_incoming_links." The semicolon added in my pull request should fix it up. Let me know if you run into any issues.

@lechat69
Copy link

Hi, I encountered this issue when I used the WP Settings API to store data in DB for my theme.
After the domain change, I loose my data, so I saw the issue in ma db.
This is an excerpt of what I found (only the data where the URLs are changed are affected) :

s:12:"opt_brochure";s:68:"http://www.mysite.com/wp-content/uploads/2013/05/file1.pdf"
s:11:"opt_contrat";s:74:"http://www.mysite.com/wp-content/uploads/2013/05/file2.pdf"
s:8:"opt_plan";s:74:"http://www.mysite.com/wp-content/uploads/2013/05/file3.pdf"
s:10:"opt_tarifs";s:66:"http://www.mysite.com/wp-content/uploads/2013/05/file4.pdf"
s:9:"opt_image";s:0:"";

It's my theme options, but, I have the same issue with NextGen Gallery Plugin.

Hope this help.

@kevindeleon
Copy link
Contributor Author

@lechat69, pretty sure @veloper was going to review things this weekend if he got a chance. Your problem looks to be due to the same issue.

@veloper
Copy link
Owner

veloper commented Jun 22, 2013

Guys I apologize for yet another delay on this but sadly I was laid off from my job last week. I've have been swamped trying to get everything in order so please bear with me while I get some things in order.

Thank you all.

@kevindeleon
Copy link
Contributor Author

@veloper wow man, sorry to hear about your job. We have some potentially bad news coming down the pipe where I work as well, so I cam empathize. Good luck with everything, and we'll keep an eye out for the update.

@kevindeleon
Copy link
Contributor Author

@veloper Hey man...just wanted to touch base to see if you had had a chance to review this pull request.

@veloper
Copy link
Owner

veloper commented Feb 28, 2014

@kevindeleon - Hey man! Sorry again for letting this sit so very long. I started to try and get PHPUnit in place for this repo during my job hunt but ended up not having enough free time to finish the implementation.

I'll try to get a very basic PHPUnit test suite setup and pulled this in over the coming weekend.

Thanks again for your persistence on this issue and your contribution to the project!

@kevindeleon
Copy link
Contributor Author

@veloper No worries...have a great weekend.

veloper added a commit that referenced this pull request Mar 2, 2014
@veloper
Copy link
Owner

veloper commented Mar 2, 2014

@kevindeleon - Would you mind checking-out the branch i've reference in to this issue and giving it a quick code review?

@veloper
Copy link
Owner

veloper commented Mar 2, 2014

@lechat69 - Thanks for your examples. I was able to reproduce the error in PHPUnit and confirm that this PR does correct the issue.

@kevindeleon
Copy link
Contributor Author

@veloper I've checked out the branch, gone through the changes and run/reviewed the test. Everything looks OK to me. Your blog link (http://dan.doezema.com/2010/04/wordpress-domain-change/) in the readme is outdated as far as usage goes for the new branch. If it's merged, you'll likely want to replace the instructions on that page with the new version's.

@veloper
Copy link
Owner

veloper commented Mar 3, 2014

@kevindeleon I've updated the blog post to match the README instructions.

Big thanks for all your help getting this sorted out!

@veloper veloper mentioned this pull request Mar 3, 2014
@veloper
Copy link
Owner

veloper commented Mar 3, 2014

Pull Request #13 (pulled in) has incorporated this PR's change. It also contains a passing test for this issue.

@veloper veloper closed this Mar 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants