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.

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

Added option for Purge Key and Custom Purge Header #5

Closed
wants to merge 6 commits into from

Conversation

jlalvarez18
Copy link

No description provided.

@maxicus
Copy link
Contributor

maxicus commented May 3, 2019

Hi,

Thank you for new functionality. Core idea is fine, but there is one problem with backwards-compatibility when host:port format is used for varnish endpoint parameter you changed.

https://github.com/W3EDGE/w3-total-cache/pull/5/files#diff-4436c0504c83d538b04e3feb771851edR118
Your change requires
http://my-varnish-host:8080/
and doesn't support
my-varnish-host:8080

Can you add support of "my-varnish-host:8080" please?
and i'll take your pull request without further modifications.

@jlalvarez18
Copy link
Author

@maxicus fixed! It now properly parsed the url and adds proper defaults.

@maxicus
Copy link
Contributor

maxicus commented May 4, 2019

Now you violated WordPress Coding Standards:
https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/
"Use real tabs and not spaces, as this allows the most flexibility across clients."

Please fix indentation.

@maxicus
Copy link
Contributor

maxicus commented May 4, 2019

Spaces style is violated too.

And there are new bugs, like
https://github.com/W3EDGE/w3-total-cache/pull/5/files#diff-4436c0504c83d538b04e3feb771851edR135 not passing your header.

@maxicus
Copy link
Contributor

maxicus commented May 8, 2019

I'm sorry you do more and more redundant modifications with each new commit. It was much better at the start.
Please post the pull request with only modifications related to the issue you fix. See the diff your pull request provides for Varnish_Flush.php file.

Also bug introduced by this pull request and mentioned above is not fixed.

@bwmarkle
Copy link
Contributor

Hi @jlalvarez18 I'm looking over the PR's for our repo and noticed this one you created over 4 months ago. I'm sorry that the PR hasn't been approved yet, I know you took the time to submit it and go back and forth in a few discussions.

I can bring this one up with the developers again. I just wanted to find out if you had any additional feedback / anything else to add?

Thanks!
- Brad

@cssjoe
Copy link
Member

cssjoe commented Dec 9, 2022

This request is very old and the code is too diverged with conflicts. If this is still an issue, then a new pull request is recommended; without extra changes for formatting, etc.

@cssjoe cssjoe closed this Dec 9, 2022
@kampol24 kampol24 mentioned this pull request Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants