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

Video XML Sitemap - Filename is deleted on update #1887

Merged
merged 8 commits into from Sep 10, 2018

Conversation

Projects
None yet
4 participants
@contactashish13
Contributor

contactashish13 commented Sep 7, 2018

adamsilverstein and others added some commits Aug 7, 2018

Merge pull request #1822 from adamsilverstein/patch-1
Use `rewrite_rules_array` as a filter
Merge pull request #1839 from semperfiwebdesign/development
Development to master for 2.7.3

@wpsmort wpsmort added this to the 2.8 milestone Sep 8, 2018

@wpsmort

This comment has been minimized.

Member

wpsmort commented Sep 8, 2018

@contactashish13 I tested this and found one problem. The sitemap name isn't being stored in the database. This means that when you export settings using the Importer / Exporter module, the value for the video sitemap filename is empty:

aiosp_video_sitemap_filename = ''

Can you correct this so that the filename is stored in the database just as it was prior to the introduction of this bug.

@contactashish13

This comment has been minimized.

Contributor

contactashish13 commented Sep 8, 2018

@wpsmort I suppose that is because you have reset options and once you do that, the filename vanishes from the database because it is not part of the options of the plugin. Can you please confirm that the sitemap name is exported as empty even when the database contains the value?

@wpsmort

This comment has been minimized.

Member

wpsmort commented Sep 9, 2018

@contactashish13 It's not because I reset the options, it's because I clicked Update Options in the Video XML Sitemap module in v2.8.3 of Pro. When you do that it wipes the filename from the database which is the bug. We need to restore the filename to the database.

@contactashish13

This comment has been minimized.

Contributor

contactashish13 commented Sep 9, 2018

@wpsmort According to the code, fields that do not exist are not touched by Update Options. Additionally, I have tested this and if the field name aiosp_video_sitemap_filename exists in the database, merely clicking Update Options does not wipe this value out. This has something to do with Reset Options. Can you please double check?

@wpsmort

This comment has been minimized.

Member

wpsmort commented Sep 9, 2018

@contactashish13 The bug happens every time I click Update Sitemap in the Video XML Sitemap module. This causes the value in the database to be removed so it looks like this - s:28:"aiosp_video_sitemap_filename";s:0:"";

To fix this bug we must set that value back to s:28:"aiosp_video_sitemap_filename";s:0:"video-sitemap";

To answer your question, this has nothing to do with the Reset button, it happens when clicking the Update button.

@contactashish13

This comment has been minimized.

Contributor

contactashish13 commented Sep 10, 2018

@michaeltorbert the code and my testing reveals that any field that is not explicitly provided in the options is not touched when Update Options/Sitemap is clicked. This means that even though we removed the field from the options, clicking Update ignores that field and updates all other fields thereby retaining its value. @wpsmort's testing results do not behave in a similar manner. I can add a hidden field so that the filename option is part of the options and gets updated along with everything else but I feel that is an overkill considering how the code is supposed to behave. Can you please let me know which option you prefer?

This bug, by the way, was introduced in the free version so the Sitemap module is also affected by this. This can be tested on that module as well.

@wpsmort

This comment has been minimized.

Member

wpsmort commented Sep 10, 2018

@michaeltorbert This PR fixes the immediate high priority bug where the Video XML Sitemap has the same URL as the normal XML Sitemap. I recommend we open a separate issue for setting the default Video XML Sitemap filename in the database which can be worked on for the next minor or major release.

I am therefore recommending that this PR be included in this release as it has been tested and is ready for code review.

@michaeltorbert michaeltorbert merged commit 10478a1 into semperfiwebdesign:2.8 Sep 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment