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

Varnish v4.1.10+ ban request not working Error Unable to detect varnish version #1490

Closed
ngongoll opened this issue May 9, 2018 · 8 comments

Comments

@ngongoll
Copy link

ngongoll commented May 9, 2018

all ban request will end with error Unable to detect varnish version when subversion of varnish 4.1 has 2 digits.

you can fix it by change the line 91 in
app/code/community/Nexcessnet/Turpentine/Model/Varnish/Admin/Socket.php
to

const REGEXP_VARNISH_VERSION = '/^varnish-(?P\d).(?P\d).(?P\d{1,2}) revision (?P[0-9a-f]+)$/';

@notimplementedyet
Copy link

notimplementedyet commented May 9, 2018

i wouldn't change it that way.
who tells you there will be no two digit minor version or three digit version sub?

if limiting the numbers i would go for something like this:
const REGEXP_VARNISH_VERSION = '/^varnish-(?P\d{1,2}).(?P\d{1,3}).(?P\d{1,2}) revision (?P[0-9a-f]+)$/;

but my prefered regex would just be just not limiting the numbers:
const REGEXP_VARNISH_VERSION = '/^varnish-(?P\d+).(?P\d+).(?P\d+) revision (?P[0-9a-f]+)$/;

@sprankhub
Copy link
Contributor

sprankhub commented May 9, 2018

Are you sure this works @ngongoll? Later, after the preg_match, the vmajor and vminor group is referenced, which you deleted:

} elseif (preg_match(self::REGEXP_VARNISH_VERSION, $bannerText[4], $matches) === 1) {
return $matches['vmajor'].'.'.$matches['vminor'];

Update: It may work, but _determineVersion does not return the correct Varnish version in this case.

Besides of that, would you mind creating a pull request?

@notimplementedyet
Copy link

created a pull request #1491

@Flyingmana
Copy link

Flyingmana commented May 10, 2018

(comment out of the off:) Could please someone do a follow up PullRequest refactoring this to not use a regex anymore? PHP has perfectly fine functions to compare versions, which are easier to read and change then a regex.

@dunkla
Copy link

dunkla commented May 23, 2018

Any updates on this?

@dunkla
Copy link

dunkla commented Jul 18, 2018

at the cost of repeating myself: any updates on this?

@miguelbalparda
Copy link
Contributor

This should be fixed in devel by #1491, care to check?

@dunkla
Copy link

dunkla commented Aug 28, 2018

Looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants