add document root check for vcl generation #25360

torhoehn · 2019-10-29T23:36:40Z

Description (*)

Manual testing scenarios (*)

Set document root to /pub
Create varnish vcl
Path to health_check.php is correct now, so Varnish is started correctly

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

m2-assistant · 2019-10-29T23:36:43Z

Hi @torhoehn. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

rodrigowebjump

Hi @torhoehn Please check my comments and fix the failing tests.

Thanks

remove line fix tests fix codestyle fix code style again

torhoehn · 2019-10-30T20:28:27Z

Hi @torhoehn Please check my comments and fix the failing tests.

Thanks

@rodrigowebjump Fixed tests, but couldn‘t find any comments.

rodrigowebjump · 2019-11-07T16:24:59Z

app/code/Magento/PageCache/Model/Varnish/VclGenerator.php

@@ -67,6 +74,7 @@ public function __construct(
        $accessList,
        $gracePeriod,
        $sslOffloadedHeader,
+        DirectoryList $directoryList,


@torhoehn The new param should come as last one.

See: https://devdocs.magento.com/guides/v2.3/contributor-guide/backward-compatible-development/
Add a new constructor param.

You're absolutely right. I fixed that.

@rodrigowebjump One of the test says, that arguments with default values must be at the end of the argument list. Should I undo my latest changes or add the DirectoryList as optional parameter?

@rodrigowebjump I think everything looks fine now. :)

rodrigowebjump · 2019-11-19T22:46:03Z

app/code/Magento/PageCache/Model/Varnish/VclGenerator.php

@@ -67,7 +74,8 @@ public function __construct(
        $accessList,
        $gracePeriod,
        $sslOffloadedHeader,
-        $designExceptions = []
+        $designExceptions = [],
+        DirectoryList $directoryList = null


Hi you also need to add the DirectoryList as optional ?DirectoryList $directoryList

@rodrigowebjump This isn't mentioned in DevDocs (https://devdocs.magento.com/guides/v2.3/contributor-guide/backward-compatible-development/#php), so im not sure about that.

public function __construct( \Old\Dependency\Interface $oldDependency, $oldRequiredConstructorParameter, $oldOptionalConstructorParameter = null, \New\Dependency\Interface $newDependency = null ) { ... $this->newDependency = $newDependency ?: \Magento\Framework\App\ObjectManager::getInstance()->get(\New\Dependency\Interface::class); }

Hi @torhoehn

This is a good practice.
I create a PR for this magento/devdocs#6033

It's is supported since PHP 7.1 (https://www.php.net/manual/en/migration71.new-features.php) and is recommended when adding a nullable param.

@rodrigowebjump Okay, I added it. :)

rodrigowebjump · 2019-11-25T13:17:53Z

@magento give me test instance

magento-engcom-team · 2019-11-25T13:18:02Z

Hi @rodrigowebjump. Thank you for your request. I'm working on Magento instance for you

magento-engcom-team · 2019-11-25T13:18:42Z

Hi @rodrigowebjump, here is your new Magento instance.
Admin access: https://pr-25360.instances.magento-community.engineering/admin
Login: admin Password: 123123q

rodrigowebjump

Hi @torhoehn

Thanks for your contribution

Pull Request state was updated. Re-review required.

torhoehn · 2020-03-25T19:23:07Z

Hi, @torhoehn.

Unfortunately, the problem described in #25360 (comment) is still reproducible.

Actual Result:
getBaseUrl() return the Base URL and the wrong path .url = "/pub/health_check.php" is written to the varnish.vcl file

@torhoehn Could you take a look?

Thanks!

@engcom-Alfa So you configured the document root instead of the Base URL pointing to 'pub' directory?

@sivaschenko Maybe we should check the request and the Base URL if one of them contains 'pub'?

sivaschenko · 2020-03-26T12:14:28Z

app/code/Magento/PageCache/Model/Varnish/VclGenerator.php

+     */
+    private function getHealthCheck() : string
+    {
+        if (strpos($this->url->getBaseUrl(), 'pub') === false) {


@torhoehn missed that during the review, sorry. If base url contains pub - health check should also contain pub and vice-versa

Suggested change

if (strpos($this->url->getBaseUrl(), 'pub') === false) {

if (strpos($this->url->getBaseUrl(), 'pub') !== false) {

Changed it.

magento-engcom-team · 2020-03-29T18:46:48Z

Hi @sivaschenko, thank you for the review.
ENGCOM-7194 has been created to process this Pull Request

engcom-Alfa

Hi @torhoehn.
During testing, we faced the issue.

Problem: If base url contains /pub health check also contain pub and everything works well. But, if base url doesn't contain /pub health check also doesn't contain 'pub' and we will get an 503 error.

Manual testing scenario:

Configure Base URL pointing to 'pub' directory ('http://magento24.loc/pub/' in my case);
Go to Admin->Stores->Configuration->Advanced->System->Full Page Cache;
Choose "Varnish Cache" from "Caching Application" drop-down and click on "Save" button;
Then click on "Export VCL for Varnish 6" button and open it;
Copy the file to the /etc/varnish/ directory;
Open home page;

Actual Result: ✔️ 200 OK

Configure Base URL to default ('http://magento24.loc/' in my case);
Repeat steps 2-4;
Copy the file to the /etc/varnish/ directory;
Open home page;

Actual Result: ✖️ 503 Backend fetch failed

@torhoehn @sivaschenko Could you take a look?

Thanks!

Pull Request state was updated. Re-review required.

torhoehn · 2020-04-02T06:03:46Z

@engcom-Alfa @sivaschenko So depending on the Base URL the health check should always contain pub, so no dynamic solution is possible?

sivaschenko · 2020-04-03T11:53:00Z

@engcom-Alfa have you changed the webserver configuration to be pointed to .../pub on step 7 of verification, is Magento accessible at http://magento24.loc/ on that step?

engcom-Alfa · 2020-04-07T14:05:13Z

@sivaschenko yes, Magento is accessible at http://magento24.loc/ on 7th step

VladimirZaets · 2020-05-01T22:14:51Z

@sivaschenko , @engcom-Alfa , @torhoehn guys, any updates on it?

torhoehn · 2020-05-29T19:45:28Z

@sivaschenko Any idea?

sivaschenko · 2020-06-03T12:24:39Z

@torhoehn do you have the same test results on your environment?

engcom-Charlie · 2020-06-30T10:47:18Z

Hi @torhoehn, I'm closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for your collaboration.

m2-assistant · 2020-06-30T10:47:20Z

Hi @torhoehn, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

torhoehn requested review from kokoc and paliarush as code owners October 29, 2019 23:36

magento-engcom-team added Component: PageCache Release Line: 2.3 labels Oct 29, 2019

m2-assistant bot added the Squashtoberfest 2019 label Oct 29, 2019

magento-engcom-team added Partner: Basecom partners-contribution Pull Request is created by Magento Partner labels Oct 29, 2019

xtrrn approved these changes Oct 30, 2019

View reviewed changes

rodrigowebjump requested changes Oct 30, 2019

View reviewed changes

ghost assigned rodrigowebjump Oct 30, 2019

ghost added the Progress: needs update label Oct 30, 2019

add document root check for vcl generation

979e059

remove line fix tests fix codestyle fix code style again

torhoehn requested a review from rodrigowebjump November 5, 2019 09:25

rodrigowebjump reviewed Nov 7, 2019

View reviewed changes

fix constructor params

eda2e37

torhoehn requested a review from rodrigowebjump November 18, 2019 22:28

rodrigowebjump requested changes Nov 19, 2019

View reviewed changes

introduce new parameter as optional parameter

88f2927

torhoehn requested a review from rodrigowebjump November 21, 2019 15:07

rodrigowebjump approved these changes Nov 25, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Nov 25, 2019

rodrigowebjump added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Nov 25, 2019

engcom-Alfa self-assigned this Nov 28, 2019

ghost removed the Progress: ready for testing label Nov 28, 2019

ghost added Progress: needs update and removed Progress: testing in progress labels Mar 25, 2020

sivaschenko reviewed Mar 26, 2020

View reviewed changes

check for base url

64905db

sivaschenko previously approved these changes Mar 29, 2020

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Mar 29, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Mar 31, 2020

engcom-Alfa requested changes Apr 1, 2020

View reviewed changes

ghost added Progress: needs update and removed Progress: testing in progress labels Apr 1, 2020

okorshenko unassigned sivaschenko May 20, 2020

engcom-Charlie closed this Jun 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add document root check for vcl generation #25360

add document root check for vcl generation #25360

torhoehn commented Oct 29, 2019 •

edited

Loading

m2-assistant bot commented Oct 29, 2019

rodrigowebjump left a comment •

edited

Loading

torhoehn commented Oct 30, 2019

rodrigowebjump Nov 7, 2019

torhoehn Nov 7, 2019

torhoehn Nov 7, 2019

torhoehn Nov 18, 2019

rodrigowebjump Nov 19, 2019

torhoehn Nov 19, 2019

rodrigowebjump Nov 19, 2019

torhoehn Nov 19, 2019

rodrigowebjump commented Nov 25, 2019

magento-engcom-team commented Nov 25, 2019

magento-engcom-team commented Nov 25, 2019

rodrigowebjump left a comment

torhoehn commented Mar 25, 2020

sivaschenko Mar 26, 2020

torhoehn Mar 26, 2020

magento-engcom-team commented Mar 29, 2020

engcom-Alfa left a comment •

edited

Loading

torhoehn commented Apr 2, 2020

sivaschenko commented Apr 3, 2020

engcom-Alfa commented Apr 7, 2020

VladimirZaets commented May 1, 2020

torhoehn commented May 29, 2020

sivaschenko commented Jun 3, 2020

engcom-Charlie commented Jun 30, 2020

m2-assistant bot commented Jun 30, 2020

	if (strpos($this->url->getBaseUrl(), 'pub') === false) {
	if (strpos($this->url->getBaseUrl(), 'pub') !== false) {

add document root check for vcl generation #25360

add document root check for vcl generation #25360

Conversation

torhoehn commented Oct 29, 2019 • edited Loading

Description (*)

Manual testing scenarios (*)

Contribution checklist (*)

m2-assistant bot commented Oct 29, 2019

rodrigowebjump left a comment • edited Loading

Choose a reason for hiding this comment

torhoehn commented Oct 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigowebjump commented Nov 25, 2019

magento-engcom-team commented Nov 25, 2019

magento-engcom-team commented Nov 25, 2019

rodrigowebjump left a comment

Choose a reason for hiding this comment

torhoehn commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magento-engcom-team commented Mar 29, 2020

engcom-Alfa left a comment • edited Loading

Choose a reason for hiding this comment

torhoehn commented Apr 2, 2020

sivaschenko commented Apr 3, 2020

engcom-Alfa commented Apr 7, 2020

VladimirZaets commented May 1, 2020

torhoehn commented May 29, 2020

sivaschenko commented Jun 3, 2020

engcom-Charlie commented Jun 30, 2020

m2-assistant bot commented Jun 30, 2020

torhoehn commented Oct 29, 2019 •

edited

Loading

rodrigowebjump left a comment •

edited

Loading

engcom-Alfa left a comment •

edited

Loading