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

FPM improvements #425

Merged
merged 16 commits into from
Jul 30, 2022
Merged

FPM improvements #425

merged 16 commits into from
Jul 30, 2022

Conversation

@iliajie
Copy link
Collaborator Author

iliajie commented Jul 29, 2022

Although, #1 may be not a good idea, as it could give a user to still edit a pool file aside from limits.

Then we should change it on phpini module itself?

@iliajie
Copy link
Collaborator Author

iliajie commented Jul 29, 2022

No, I have a better idea. Will add a commit to the PR after the walk..

@iliajie
Copy link
Collaborator Author

iliajie commented Jul 30, 2022

I think I fixed all known PHP related issues (with Apache) ...

save_phpmode.cgi Outdated
@@ -11,7 +11,8 @@ $canv = &can_edit_phpver($d);
$can || $canv || &error($text{'phpmode_ecannot'});
&require_apache();
$p = &domain_has_website($d);

@modes = &supported_php_modes($d);
($newmode) = grep { $in{'mode'} =~ /^$_/ } @modes;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use a regexp here?

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 is because (most probably a theme bug, which I will check on a bit later today), when you hit enter to submit a form it adds mode two time to the payload which makes $in{'mode'} set to something like fpmfpm ..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hold on, we should fix that bug in instead of working around it here!!

Copy link
Collaborator Author

@iliajie iliajie Jul 30, 2022

Choose a reason for hiding this comment

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

Hold on, we should fix that bug in instead of working around it here!!

Absolutely, although this was before I realized that it was a bug! Also, this a small improvement to the code here anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Assuming that bug is fixed, can't this line just be $newmode = $in{'mode'};

Copy link
Collaborator Author

@iliajie iliajie Jul 30, 2022

Choose a reason for hiding this comment

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

Yes, let's do it. Fixed.

Assuming that bug is fixed,

Yes, I have been fighting with it today for many hours to make it right on the theme side ... finishing final tests ..

$defchildren = 9999;
}
else {
$defchildren = $defchildren >= 5 ? $defchildren : 5;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this limit of 5 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why this limit of 5 ?

Check this updated help, it explains it - help/phpmode_children.html

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will setting it to less than 5 create an invalid config that can't be loaded by PHP?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, unless pm.max_spare_servers also manually decrease. But we have it hard coded upon pool creation time .. so I decided to make it easy but not obscure ..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, makes sense!

@jcameron jcameron merged commit 92849aa into master Jul 30, 2022
@iliajie iliajie deleted the dev/php-fpm-improvements branch July 30, 2022 21:37
@iliajie
Copy link
Collaborator Author

iliajie commented Jul 30, 2022

Thanks. I will create another PR if needed when taking another deep look into it ..

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

Successfully merging this pull request may close these issues.

3 participants