-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Report fatal error if JIT cannot be enabled #12403
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
Conversation
Emitting a warning could make sense. A fatal error is not something that should be introduced in a patch. We wouldn't want websites to go down because somebody updates their system. I think a |
Sending this to master then :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good for master.
@danog Can you have a look at the CI failures? They are legit. |
5c2e4e9
to
3db87ea
Compare
@iluuu1994 Fixed everything, should be good to merge! |
@iluuu1994 Did a little whoopsie with the JIT config branch, re-pushed this one, should be OK now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look ok. I do feel a bit like completely refusing to start PHP is a bit harsh. Upgrading a server to a newer version of PHP might cause down times and require digging into log files to discover the issue. This is something that may be missed during development because config files of the server may obviously diverse.
If @dstogov thinks it's ok then I won't object. I would personally prefer warnings (that actually get emitted with the default configuration).
@danog Can you maybe send a short e-mail to the mailing list asking for feedback? I'm ok with this change if nobody complains. |
@iluuu1994 I can, but I'm not sure it is entirely needed to be frank, this is a change which simply makes (generally rare) configuration mistakes more evident, plus it would ship on php 8.4 and is mentioned in the upgrade notes, I don't see why would anyone be against a change which simply unhides existing issues, but if you insist I can drop an email. |
As mentioned, people may be upgrading PHP versions on the server directly. Of course, I agree that that's not good practice, but likely something lots of people do anyway. Nonetheless, a JIT misconfiguration seems like a unjustified reason to completely refuse launching the process and taking down the website, at least in my opinion. I don't particularly care for myself because I don't really host any websites anymore. But I could imagine other people having similar feelings. If you send an e-mail and nobody complains, nobody can come back and shift the blame on anybody. |
@iluuu1994 Done, hasn't appeared on externals yet for some reason, did you get it? BTW could you merge #12406 in the meantime? |
@danog I received the e-mail. I think externals.io sometimes takes a while to catch up. |
Hi @iluuu1994, two weeks have passed and it seems there were no complaints from the mailing list, do you think you can merge this? :) |
@danog Fair enough. 🙂 I'll merge this on Monday at the latest. |
@iluuu1994 Awesome! Could you also merge #12406? |
No description provided.