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.

Sign up for GitHub

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

Jump to bottom

Fix problem with gzip option #218

Merged
merged 10 commits into from
Feb 28, 2020
Merged

Fix problem with gzip option #218

merged 10 commits into from
Feb 28, 2020

Conversation

delda
Copy link
Contributor

@delda delda commented Dec 19, 2019

This PR fixes two issues: #140 and #197

If you generate two distinct sitemap sections with gzip option (for example static and dynamic) on the second result your sitemap.xml contains only the last link with .gz extention.
This is the example result:

<?xml version="1.0" encoding="UTF-8"?>
<sitemapindex xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
              xsi:schemaLocation="http://www.sitemaps.org/schemas/sitemap/0.9 http://www.sitemaps.org/schemas/sitemap/0.9/siteindex.xsd"
              xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">
    <sitemap>
        <loc>https://www.example.com/sitemap/sitemap.dynamic.xml.gz</loc>
        <lastmod>2019-12-19T16:57:20-05:00</lastmod>
    </sitemap>              
    <sitemap>
        <loc>https://www.example.com/sitemap/sitemap.static.xml</loc>
        <lastmod>2019-12-19T16:55:50-05:00</lastmod>
    </sitemap>
</sitemapindex>

With this PR, the software "remember" the previous gz extensions written into sitemap.xml
and the final result will do:

<?xml version="1.0" encoding="UTF-8"?>
<sitemapindex xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
              xsi:schemaLocation="http://www.sitemaps.org/schemas/sitemap/0.9 http://www.sitemaps.org/schemas/sitemap/0.9/siteindex.xsd"
              xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">
    <sitemap>
        <loc>https://www.example.com/sitemap/sitemap.dynamic.xml.gz</loc>
        <lastmod>2019-12-19T16:57:20-05:00</lastmod>
    </sitemap>              
    <sitemap>
        <loc>https://www.example.com/sitemap/sitemap.static.xml.gz</loc>
        <lastmod>2019-12-19T16:55:50-05:00</lastmod>
    </sitemap>
</sitemapindex>

yann-eugone
yann-eugone requested changes Jan 2, 2020
View reviewed changes
Copy link
Member

@yann-eugone yann-eugone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a complement, can you please write a test for this ? So it is enforced not to break in the future.

Service/Dumper.php Show resolved Hide resolved
Service/Dumper.php Outdated Show resolved Hide resolved
Service/Dumper.php Outdated Show resolved Hide resolved
Service/Dumper.php Outdated Show resolved Hide resolved
@yann-eugone yann-eugone self-assigned this Jan 2, 2020
Davide Dell'Erba added 7 commits January 2, 2020 23:25
Missing a blank line before return statement
81f4521
Word "extention" replaced with "extension"
fae6a48
isset() already return a boolean
ce30828
not strict return types required
4a5d10b
proper phpdoc
2ec3b04
Added two tests on gzip option
d9fcba9
Error correction:
ea70514 
Calling "Symfony\Bundle\FrameworkBundle\Test\WebTestCase::createClient()" while a kernel has been booted is deprecated since Symfony 4.4 and will throw in 5.0, ensure the kernel is shut down before calling the method.
@delda delda requested a review from yann-eugone January 4, 2020 23:17
yann-eugone
yann-eugone reviewed Jan 6, 2020
View reviewed changes
Copy link
Member

@yann-eugone yann-eugone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, thank you for this contribution, last effort until this get merged

Tests/Sitemap/SitemapZgipOptionTest.php Outdated Show resolved Hide resolved
Tests/Sitemap/SitemapZgipOptionTest.php Outdated Show resolved Hide resolved
Tests/Sitemap/SitemapZgipOptionTest.php Outdated Show resolved Hide resolved
Tests/fixtures/sitemap_with_gz.xml Outdated Show resolved Hide resolved
Tests/fixtures/sitemap_without_gz.xml Outdated Show resolved Hide resolved
yann-eugone
yann-eugone requested changes Jan 6, 2020
View reviewed changes
Copy link
Member

@yann-eugone yann-eugone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are having trouble with tests, please tell us, maybe someone can help

@delda delda requested a review from yann-eugone January 9, 2020 17:19
yann-eugone
yann-eugone approved these changes Jan 15, 2020
View reviewed changes
Copy link
Member

@yann-eugone yann-eugone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this nice catch and your efforts with tests.

@yann-eugone yann-eugone merged commit 05d903f into prestaconcept:master Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yann-eugone yann-eugone yann-eugone approved these changes

Assignees

@yann-eugone yann-eugone

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@delda @yann-eugone