-
Notifications
You must be signed in to change notification settings - Fork 15
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
fixed Issue #17 support URL suffix. #18
base: master
Are you sure you want to change the base?
Conversation
add support suffix url fixed bug '/' ending VSF URL.
add php 7.4 and magento 2.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for you contribution!
@@ -57,14 +57,16 @@ public function __construct( | |||
ProductCollection $productCollection, | |||
DirectoryList $directoryList, | |||
SitemapFactory $sitemapFactory, | |||
\Magento\Framework\App\Config\ScopeConfigInterface $appConfigScopeConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you import the class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on my development environment (Magento 2.4) this code is worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you still import it so the coding style matches the original?
@@ -87,7 +89,7 @@ public function execute() : void | |||
$this->addProductsToSitemap(); | |||
|
|||
// Generate | |||
$this->sitemap->createSitemapIndex($domain, 'Today'); | |||
$this->sitemap->createSitemapIndex($domain . "/", 'Today'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it needed to add a "/" here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as current will generate 2 files.
- when VSF URL in domain name end with '/' sitemap-index.xml will corrected but sitemap.xml will have '//'.
- when VSF URL domain name without end with '/' sitemap-index.xml will incorrect but site sitemap.xml will corrected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, so in that case it is better that we just force the user to enter the url with a '/' suffix. We could do this https://github.com/Vendic/magento2-vuestorefront-xmlsitemap/blob/master/etc/adminhtml/system.xml#L13-L16 using the <validate>
tag.
@@ -57,14 +57,16 @@ public function __construct( | |||
ProductCollection $productCollection, | |||
DirectoryList $directoryList, | |||
SitemapFactory $sitemapFactory, | |||
\Magento\Framework\App\Config\ScopeConfigInterface $appConfigScopeConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you still import it so the coding style matches the original?
fixed issue #17
this used on my environment is VSF 1.9 + Magento 2.4