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

Fixes tabs for geo-location RSS feeds #13801

Merged
merged 4 commits into from
Oct 25, 2019
Merged

Fixes tabs for geo-location RSS feeds #13801

merged 4 commits into from
Oct 25, 2019

Conversation

ChaosExAnima
Copy link
Contributor

@ChaosExAnima ChaosExAnima commented Oct 22, 2019 •
edited
Loading

Fixes #12051

Changes proposed in this Pull Request:

  • This resolves an issue where the RSS feeds with geo-location had invalid whitespacing, causing parser issues.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • This is a bug fix.

Testing instructions:

  • Visit RSS feed, and view namespaces.

Before:
Screenshot from 2019-10-22 11-51-52

After:
Screenshot from 2019-10-22 11-50-44

Proposed changelog entry for your changes:

  • Fixed spacing for RSS geo-location namespaces.

@ChaosExAnima ChaosExAnima self-assigned this Oct 22, 2019
@jetpackbot
Copy link

jetpackbot commented Oct 22, 2019 •
edited
Loading

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 731198c

@ChaosExAnima ChaosExAnima requested a review from a team October 22, 2019 15:49
ChaosExAnima
ChaosExAnima commented Oct 22, 2019
View reviewed changes
modules/geo-location/class.jetpack-geo-location.php Outdated
@@ -249,7 +249,8 @@ public function register_rss_hooks() {
* Add the georss namespace during RSS generation.
*/
public function rss_namespace() {
echo PHP_EOL . 'xmlns:georss="http://www.georss.org/georss" xmlns:geo="http://www.w3.org/2003/01/geo/wgs84_pos#"' . PHP_EOL;
echo 'xmlns:georss="http://www.georss.org/georss\"' . PHP_EOL . "\t";
echo 'xmlns:geo="http://www.w3.org/2003/01/geo/wgs84_pos#"' . PHP_EOL . "\t";

This comment was marked as outdated.

Sign in to view

@ChaosExAnima ChaosExAnima force-pushed the fix/12051-georss-whitespacing branch from 0e24d21 to 5292d0f Compare October 22, 2019 15:51
@ChaosExAnima ChaosExAnima added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Oct 22, 2019
ChaosExAnima
ChaosExAnima commented Oct 22, 2019
View reviewed changes
modules/geo-location/class.jetpack-geo-location.php
@@ -249,7 +249,8 @@ public function register_rss_hooks() {
* Add the georss namespace during RSS generation.
*/
public function rss_namespace() {
echo PHP_EOL . 'xmlns:georss="http://www.georss.org/georss" xmlns:geo="http://www.w3.org/2003/01/geo/wgs84_pos#"' . PHP_EOL;
echo PHP_EOL . "\t" . 'xmlns:georss="http://www.georss.org/georss\"';
echo PHP_EOL . "\t" . 'xmlns:geo="http://www.w3.org/2003/01/geo/wgs84_pos#"';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split apart for readability.

Copy link

Choose a reason for hiding this comment

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

fix/12051-georss-whitespacing

@ChaosExAnima ChaosExAnima added this to the 7.9 milestone Oct 22, 2019
@jeherve jeherve added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Oct 22, 2019
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 22, 2019
@ChaosExAnima ChaosExAnima added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Oct 22, 2019
jeherve
jeherve previously approved these changes Oct 23, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good to me, it should be good to merge!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 23, 2019
kraftbj
kraftbj reviewed Oct 24, 2019
View reviewed changes
modules/geo-location/class.jetpack-geo-location.php Outdated
@@ -249,7 +249,9 @@ public function register_rss_hooks() {
* Add the georss namespace during RSS generation.
*/
public function rss_namespace() {
echo PHP_EOL . 'xmlns:georss="http://www.georss.org/georss" xmlns:geo="http://www.w3.org/2003/01/geo/wgs84_pos#"' . PHP_EOL;
echo PHP_EOL . "\t" . 'xmlns:georss="http://www.georss.org/georss\"';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the trailing backslash needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, good catch! Fixed in 731198c

@kraftbj
Copy link
Contributor

kraftbj commented Oct 25, 2019

Test failure looks like an unrelated fragile sync test that failed on master/php7.4. Rerunning.

@kraftbj kraftbj merged commit c97bfc3 into master Oct 25, 2019
@kraftbj kraftbj deleted the fix/12051-georss-whitespacing branch October 25, 2019 14:22
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 25, 2019
jeherve added a commit that referenced this pull request Oct 28, 2019
@jeherve
Changelog: add #13801
c4294ed
jeherve added a commit that referenced this pull request Oct 29, 2019
@jeherve
7.9: Changelog (#13608)
6cd1621 
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
@Automattic Automattic deleted a comment from Ael02 Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ael02 Ael02 Ael02 left review comments

@kraftbj kraftbj kraftbj approved these changes

@jeherve jeherve jeherve left review comments

Assignees

@ChaosExAnima ChaosExAnima

Labels
[Feature] Geo Location [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
7.9
Development

Successfully merging this pull request may close these issues.

Geo-location: improve display of xmlns:georss attribute
6 participants
@ChaosExAnima @jetpackbot @kraftbj @jeherve @Ael02 @matticbot