Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

danog
Copy link
Contributor

@danog danog commented Oct 10, 2023

No description provided.

@iluuu1994
Copy link
Member

iluuu1994 commented Oct 10, 2023

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 ACCEL_LOG_WARNING would be uncontroversial 🙂

@danog danog changed the title Report error if JIT cannot be enabled Report fatal error if JIT cannot be enabled Oct 10, 2023
@danog danog changed the base branch from PHP-8.1 to master October 10, 2023 15:43
@danog
Copy link
Contributor Author

danog commented Oct 10, 2023

Sending this to master then :)

Copy link
Member

@dstogov dstogov left a 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.

@iluuu1994
Copy link
Member

@danog Can you have a look at the CI failures? They are legit.

@danog danog force-pushed the report_error branch 3 times, most recently from 5c2e4e9 to 3db87ea Compare November 15, 2023 13:29
@danog
Copy link
Contributor Author

danog commented Nov 15, 2023

@iluuu1994 Fixed everything, should be good to merge!

@danog
Copy link
Contributor Author

danog commented Nov 15, 2023

@iluuu1994 Did a little whoopsie with the JIT config branch, re-pushed this one, should be OK now :)

Copy link
Member

@iluuu1994 iluuu1994 left a 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).

@iluuu1994
Copy link
Member

@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.

@danog
Copy link
Contributor Author

danog commented Nov 17, 2023

@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.

@iluuu1994
Copy link
Member

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.

@danog
Copy link
Contributor Author

danog commented Nov 17, 2023

@iluuu1994 Done, hasn't appeared on externals yet for some reason, did you get it?

BTW could you merge #12406 in the meantime?
As per my last comment there, I believe up to 4x speedups on 4-core GHA workers and a simplified reprodicible script runnable on other CIs would be pretty neat to have :)

@iluuu1994
Copy link
Member

@danog I received the e-mail. I think externals.io sometimes takes a while to catch up.

@danog
Copy link
Contributor Author

danog commented Nov 25, 2023

Hi @iluuu1994, two weeks have passed and it seems there were no complaints from the mailing list, do you think you can merge this? :)

@iluuu1994
Copy link
Member

@danog Fair enough. 🙂 I'll merge this on Monday at the latest.

@danog
Copy link
Contributor Author

danog commented Nov 26, 2023

@iluuu1994 Awesome! Could you also merge #12406?

@iluuu1994 iluuu1994 closed this in fde41bc Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants