Skip to content

Conversation

@mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Mar 28, 2025

fixes: #3442

seems kinda hacky i dont know how else we can easily make the output parseable.
I guess we could turn of deprecations and all error handling for that request but i dindt want to start the fun to make these parameters work cross platform (windows) so here is an easy fix.

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@kdambekalns
Copy link
Member

Slightly related, as it eases finding such issues: #3441

Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

I am not convinced we should do that, there could be other output apart from deprecations that should block using the binary, also like if there are deprecations maybe use a log level that silences them if you want to use that php version?

@mhsdesign
Copy link
Member Author

mhsdesign commented Nov 6, 2025

This issue just popped up in slack when there is an error like OPcache already loaded

I think we should reconsider if the check really has to be done at (production) runtime or if this should be part of the (one time) neos setup (./flow setup and /setup endpoint). Adding this validation to the setup would allow us to give more verbose output to the user. Like if the php binary emits odd stuff i want to know about it ^^. But if this all has to be be encapsulated when the actual runtime code already attempts to start a php subprocess its a little late for this guidance. (Especially if this is deployed already wrongly)

So i think we should try to remove the (expensive) php validation here from the Scripts and instead fully put the load to the setup with more verbose information. The only problem is that we need to see if we want people to have the neos setup package installed on the production server to see possible problems ... maybe hidden by login after all...

@dlubitz
Copy link
Contributor

dlubitz commented Nov 6, 2025

But in the setup this doesn't help at all. Mostly I run into this during changing the PHP versions in already productive running systems and you forgot to change the binary path in the config.

@skurfuerst
Copy link
Member

Hmmm, I think it is problematic to remove this check if we cannot hint to the user that something is wrong. In fact we would need to point the user to the setup in error cases; otherwise they will not find it... I need to think it through more carefully though.

@kdambekalns
Copy link
Member

when there is an error like OPcache already loaded

With #3441 the cause should be clearly visible. And then you can just fix it, no?

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.

BUG: PHP binary check may report false negatives

5 participants