Skip to content

set xmlns for sitemap-news also with gnews_single mode#91

Merged
yellowled merged 1 commit intos9y:masterfrom
hannob:master
Mar 1, 2019
Merged

set xmlns for sitemap-news also with gnews_single mode#91
yellowled merged 1 commit intos9y:masterfrom
hannob:master

Conversation

@hannob
Copy link
Copy Markdown
Contributor

@hannob hannob commented Mar 1, 2019

This should fix the errors I reported here:
https://board.s9y.org/viewtopic.php?f=3&t=24196

The problem in the code is this: The sitemap xmlns was included conditionally if $gnewsmode is set. However that is only the case for the separate gnews sitemap.
In the case of gnews_single - where the news items are included in the main sitemap - it's not. But it is still required.

There's a check for that and it sets $this->gnewsmode, but after that block, so I had to switch those code blocks around and change the if($gnewsmode) to if($this->gnewsmode).

Please merge.

@yellowled yellowled requested review from garvinhicking and onli and removed request for garvinhicking March 1, 2019 14:08
@yellowled yellowled merged commit 92ad83b into s9y:master Mar 1, 2019
@yellowled
Copy link
Copy Markdown
Member

@hannob Thx! 👍

@onli
Copy link
Copy Markdown
Member

onli commented Mar 1, 2019

In fact I missed something: Changes like this should always also raise the version number. Especially important for spartacus plugins to trigger upgrades. I will do that in a separate commit now.

@yellowled
Copy link
Copy Markdown
Member

@onli Can you please also document @hannob s changes in NEWS while you're at it?

@onli
Copy link
Copy Markdown
Member

onli commented Mar 1, 2019

Do we document plugin changes there? But I added a line to the plugin changelog: ca327e3

@yellowled
Copy link
Copy Markdown
Member

No, I just totally missed that this is in additional_plugins, sorry.

@onli
Copy link
Copy Markdown
Member

onli commented Mar 1, 2019

I did at first as well^^ This plugin might as well be a core plugin.

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.

3 participants