Skip to content

Commit

Permalink
chained freature API has changed
Browse files Browse the repository at this point in the history
  • Loading branch information
@jcameron
jcameron committed Dec 16, 2022
1 parent 43279c8 commit ec5e75d
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions virtual_feature.pl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -558,14 +558,14 @@ sub feature_can_reset
# Nginx SSL will be activated if a regular website is
sub feature_can_chained
{
return 1;
return ('virtualmin-nginx');
}

# Returns 1 if the regular website is enabled and on by default
sub feature_chained
{
my ($d, $oldd) = @_;
if ($virtual_server::config{'plugins_inactive'} =~ /\Q$module_name\E/) {
if (&indexof($module_name, @virtual_server::plugins_inactive) >= 0) {
# Not in auto mode
return undef;
}
Expand Down

19 comments on commit ec5e75d

@iliajie
Copy link
Contributor

Choose a reason for hiding this comment

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

Jamie, I discovered another bug in regards of SSL chained feature - if you install Ghost (when Nginx config is changed), it adds proxy pass correctly, however SSL records are getting dropped, i.e.:

listen 10.211.55.74:443 ssl;
listen [fdb2:2c26:f4e4:0:21c:42ff:feb8:898f]:443 ssl;
ssl_certificate /etc/ssl/virtualmin/167231608114002/ssl.cert;
ssl_certificate_key /etc/ssl/virtualmin/167231608114002/ssl.key;

.. from the end of the config ..

Also, if you later go to Edit Virtual Server and try to enable Nginx SSL feature, save but nothing gets enabled. Nginx SSL is set as Default? in Features and Plugins page.

@jcameron
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's odd - installing Ghost shouldn't enable or disable SSL features at all!

@iliajie
Copy link
Contributor

@iliajie iliajie commented on ec5e75d Jan 5, 2023

Choose a reason for hiding this comment

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

It seems as related, and hopefully helps to fix it:

image

@jcameron
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That looks like a bug! Was an SSL Nginx website enabled for this domain before running the disable-feature command?

@iliajie
Copy link
Contributor

@iliajie iliajie commented on ec5e75d Jan 9, 2023

Choose a reason for hiding this comment

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

Yes, it was enabled.

@jcameron
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should fix it : f21f1fa

@iliajie
Copy link
Contributor

@iliajie iliajie commented on ec5e75d Jan 9, 2023

Choose a reason for hiding this comment

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

Thanks. I see now. The actual fix is virtualmin/virtualmin-gpl@222770b. It means we won't be able to release new Virtualmin Nginx until Virtualmin 7.6 goes out.

@iliajie
Copy link
Contributor

@iliajie iliajie commented on ec5e75d Jan 9, 2023

Choose a reason for hiding this comment

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

Wait, no! Actually we can make a new Nginx plugins releases, because it works with this simple patch already, as we test for if (defined($c)){} in current Virtualmin release!

But, for the love of God, this is very damn odd and bug-prone that Perl internally returns an empty string from a sub without return and wants explicit return undef. This is bananas! I wonder if this is some kind of a burden of the past, a very first Perl versions?

@jcameron
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Either one of those patches will fix it, but we can release a new Nginx SSL plugin ASAP (and I tagged a new version already).

@iliajie
Copy link
Contributor

@iliajie iliajie commented on ec5e75d Jan 9, 2023

Choose a reason for hiding this comment

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

Yes, I realize that either of those patches fixes it. Although my question was rather different.

By the way, I have just tagged Nginx plugin, as we need to do a new release of it too. @swelljoe Joe, you can go with both Virtualmin Nginx and Virtualmin Nginx SS releases.

@jcameron
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I don't like that Perl returns work that way! Best we can do is just avoid it ...

@iliajie
Copy link
Contributor

@iliajie iliajie commented on ec5e75d Jan 9, 2023

Choose a reason for hiding this comment

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

Yeah, and what's worse, it returns different results in scalar and list context! 🤯 But also, if I remember correctly Joe once mentioned that return undef is not welcomed by Perl::Critic either. I personally consider returning return undef is a safest alternative.

@jcameron
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Really? I use return undef all the time..

@iliajie
Copy link
Contributor

@iliajie iliajie commented on ec5e75d Jan 9, 2023

Choose a reason for hiding this comment

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

Yeah, but return undef makes sense and must be the default behaviour in Perl, as return different results depending on the context is prone to hard to debug bugs.

@jcameron
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder is there a way we can detect missing return statements?

@iliajie
Copy link
Contributor

@iliajie iliajie commented on ec5e75d Jan 9, 2023

Choose a reason for hiding this comment

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

What do you mean by "detect"? Detect for which purposes?

@jcameron
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean, some kind of linter that can analyze the code and report potential bugs like this.

@iliajie
Copy link
Contributor

@iliajie iliajie commented on ec5e75d Jan 9, 2023

Choose a reason for hiding this comment

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

I think it will be pretty difficult considering that a function can have multiple returns. Also, I am not sure if linter will help a lot here because I don't think a linter could properly detect the context.

We just need to make sure, when writing the code, that function always return something..

@jcameron
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In some languages like Go this kind of static analysis is possible, but Perl is dynamic enough that it might not be..

Please sign in to comment.