Skip to content

Conversation

aschreck
Copy link
Contributor

Adds logic to deliver the appropriate magento vcl based upon the varnish image of the account making the request. Tested with Varnish 4, 5, and 6 instances alongside other proxies.

Helper/Data.php Outdated
$applicationFactory->save();
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to add comment block that defines what the function does and what parameters it takes. Follow guide from other public functions.

$proxy_image = $this->helper->getProxyImage($account_id, $application_id);

/** Extract the generated VCL code appropriate for their version*/
if (strpos($proxy_image, "4" !== false)){
Copy link
Contributor

Choose a reason for hiding this comment

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

strpos would capture 5.4.2 or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since varnish 4.* needs one vcl file and all the other currently released versions(5.* and 6.*) need the other, I'm stripping off all digits but the major release. Varnish 5.4.2 is hitting this if-block as "varnish:5". Feel free to suggest if you see a better way.

$application_id = $this->state->getApplicationId();
/** @var string $environment_name */
$environment_name = $this->state->getEnvironmentName();
/** @var string $proxy_image*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This code block apparently duplicated here and in UpdateVclCommand.php seems like an opportunity to refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@aschreck aschreck merged commit a882af9 into master May 23, 2018
@aschreck aschreck deleted the upgrade-varnish-support branch May 23, 2018 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants