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

Issue #2802, #1146: Fixing sitemap generation folder #9094

Merged
merged 10 commits into from
Apr 19, 2017
Merged

Issue #2802, #1146: Fixing sitemap generation folder #9094

merged 10 commits into from
Apr 19, 2017

Conversation

JosephMaxwell
Copy link
Contributor

@JosephMaxwell JosephMaxwell commented Apr 2, 2017 •
edited by okorshenko
Loading

Description

Magento assumes for the above tickets that robots.txt and the sitemap paths are to be found in the root directory. To get the sitemap to be generated in the pub folder (per Magento's recommendation), you need to set the base directory to be at least pub/. This resolves the issue by adding new configuration to app/etc/env.php:

directories/
    document_root_is_pub (bool)

The default keeps backward compatibility: if the value is not specified, original functionality remains (use of the root directory). If you specify document_root_is_pub and set it to true, the sitemap is located in the correct directory.

Fixed Issues

This is a fix for:

  1. Sitemap generation in wrong folder when vhost is connected to pub folder #2802
  2. A few issues when you use /pub as DocumentRoot #1146

Manual testing scenarios

Before:

  1. Ensure that apache / nginx has the document root set to use /pub.
  2. Create a sitemap.
  3. Click the generate link.
  4. Try to access it: you will get a 404 error.

After:

  1. Sitemap should be generated, and you will not get a 404 error.

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Apr 2, 2017 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@okorshenko okorshenko changed the title 2802: Fixing sitemap generation folder Issue #2802, #1146: Fixing sitemap generation folder Apr 2, 2017
okorshenko
okorshenko suggested changes Apr 2, 2017
View reviewed changes
app/code/Magento/Config/Model/Config/Backend/Admin/Robots.php Outdated
@@ -41,10 +42,13 @@ public function __construct(
\Magento\Framework\Filesystem $filesystem,
\Magento\Framework\Model\ResourceModel\AbstractResource $resource = null,
\Magento\Framework\Data\Collection\AbstractDb $resourceCollection = null,
\Magento\Config\Model\Config\Reader\Source\Deployed\DocumentRoot $documentRoot = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

please, update doc block

Copy link
Member

Choose a reason for hiding this comment

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

done

app/code/Magento/Config/Model/Config/Reader/Source/Deployed/DocumentRoot.php Outdated
*/
class DocumentRoot
{
private $config;
Copy link
Contributor

Choose a reason for hiding this comment

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

add doc block here

Copy link
Member

Choose a reason for hiding this comment

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

done

app/code/Magento/Sitemap/Block/Adminhtml/Grid/Renderer/Link.php Outdated
@@ -34,10 +41,13 @@ public function __construct(
\Magento\Backend\Block\Context $context,
\Magento\Sitemap\Model\SitemapFactory $sitemapFactory,
\Magento\Framework\Filesystem $filesystem,
\Magento\Config\Model\Config\Reader\Source\Deployed\DocumentRoot $documentRoot = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

please, update doc block

Copy link
Member

Choose a reason for hiding this comment

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

done

@okorshenko okorshenko self-assigned this Apr 2, 2017
@okorshenko okorshenko added this to the April 2017 milestone Apr 2, 2017
@JosephMaxwell
Copy link
Contributor Author

@okorshenko: we updated the PR per your comments.

@magento-team magento-team merged commit 3cc0e5a into magento:develop Apr 19, 2017
magento-team pushed a commit that referenced this pull request Apr 19, 2017
MAGETWO-67054: Issue #2802, #1146: Fixing sitemap generation folder #…
00a4904 
…9094

 - restored original file content
magento-team pushed a commit that referenced this pull request Apr 19, 2017
MAGETWO-67054: Issue #2802, #1146: Fixing sitemap generation folder #…
1e1a5ad 
…9094

 - fixed static tests
magento-team pushed a commit that referenced this pull request Apr 19, 2017
MAGETWO-67054: Issue #2802, #1146: Fixing sitemap generation folder #…
34d1557 
…9094

 - fixed static tests
magento-team pushed a commit that referenced this pull request Apr 19, 2017
MAGETWO-67054: Issue #2802, #1146: Fixing sitemap generation folder #…
01fe5d7 
…9094

 - fixed static tests
@magento-team
Copy link
Contributor

@JosephMaxwell thank you for your contribution. Your Pull Request has been successfully merged

@okorshenko okorshenko mentioned this pull request Apr 19, 2017
Closed
@skovalenk
Copy link
Contributor

skovalenk commented Apr 28, 2017 •
edited
Loading

@magento/community-gatekeepers @JosephMaxwell how other developers should learn about this configuration in env.php
ENV.PHP is file that should be generated, so by this logic there should be command which allows to switch root folder.
Or PR to documentation with this feature should be added.

@okorshenko
Copy link
Contributor

Hi @sereban
We are working on documentation for this functionality.
Thank you

@avoelkl
Copy link
Contributor

avoelkl commented Nov 17, 2017

Hi @JosephMaxwell,
I just came across this PR because I was wondering if there was already a fix for the sitemap.xml issue - and there is! Thank you :)

I added the following to my env.php:

    'directories' =>
        array(
            'document_root_is_pub' => true
        ),

After clicking "Generate" the URL still contains /pub/ but the sitemap works with and without /pub:

  • /pub/sitemap/cz/cs/sitemap.xml
  • /sitemap/cz/cs/sitemap.xml

Is this the expected result?

@magento-engcom-team magento-engcom-team added bugfix Fixed in 2.2.x The issue has been fixed in 2.2 release line PROD labels Nov 17, 2017
@magento-engcom-team magento-engcom-team added Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release SEO labels Nov 17, 2017
@avstudnitz
Copy link
Contributor

@okorshenko What about said documentation? I can't find it...

@webspeaks
Copy link

@okorshenko Any documentation available? This issue still exists in 2.2.5.

@jalogut
Copy link
Contributor

jalogut commented Sep 13, 2018

It would be great to find documentation about that in the dev docs. I only found that:

Is the 'document_root_is_pub' => true needed for something else too?

@Bartlomiejsz Bartlomiejsz mentioned this pull request Mar 29, 2019
Closed
@ihor-sviziev ihor-sviziev mentioned this pull request Mar 29, 2019
Closed
@OvalMedia
Copy link

2.2.9 and the issue persists. Nginx/Apache Docroot is set to /pub folder.
Sitemap entry in the robots.txt still looks like this:
"Sitemap: https://www.example.com/pub/sitemap.xml"

document_root_is_pub in env.php does not make any difference.

What am I missing here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rodrigowebjump rodrigowebjump rodrigowebjump left review comments

@okorshenko okorshenko okorshenko approved these changes

Assignees

@okorshenko okorshenko

Labels
bugfix Fixed in 2.2.x The issue has been fixed in 2.2 release line Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release
Projects
None yet
Milestone
April 2017
Development

Successfully merging this pull request may close these issues.