Skip to content

fix: remove PHP cache validation disabling example #12079

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joshtrichards
Copy link
Member

☑️ Resolves

The way this is presented it seems people skim and enter this example blindly without reading the surrounding text.

We don't want people thinking we recommend this for all environments.

If one wishes to disable validation, that's fine (as noted in the text), but they presumably know what they're doing and can figure it out from the PHP manual on their own.

I think @kesselb hinted about removing this part during the last edit round of this area. ;-)

🖼️ Screenshots

@joshtrichards
Copy link
Member Author

/backport to stable29

@joshtrichards
Copy link
Member Author

/backport to stable28

@kesselb
Copy link
Contributor

kesselb commented Jul 31, 2024

Any Server/app upgrades or changes to config.php will then require restarting PHP (or otherwise manually clearing the cache or invalidating this particular script).

Should we remove the quoted part as well?

I think @kesselb hinted about removing this part during the last edit round of this area. ;-)

Yep. As you wrote, people just copy it because it's on the server tuning page and have no idea about the impact.

@MichaIng
Copy link
Member

MichaIng commented Jul 31, 2024

Hmm, do we really want to remove valuable information just because some people do not do what docs are made for: reading?

I personally actually do recommend disabling PHP revalidation for serious production Nextcloud systems where you want to max out performance = user experience as much as possible. I personally have a little command which clears config.php from OPcache via opcache-gui web API, for cases where I do not want to restart PHP. When upgrading Nextcloud from console, I restart PHP anyway, as the downtime is there already.

No problem with making warnings clearer, putting the whole info into a warning box, whatever, but we are not Apple here (sorry for the bias/cliche 😉) where we offer just one failsafe default to prevent users (and we are talking about "admins" here) from configuring things wrong. People do things wrong, and they can learn from this, especially as admin valuable/essential.

Just my two cents 😉.

Should we remove the quoted part as well?

Makes sense then, as this absolutely belongs to disabling revalidation.

@rakekniven
Copy link
Member

Is there now a consensus here?

Signed-off-by: Josh <josh.t.richards@gmail.com>
@kesselb kesselb force-pushed the fix/admin-php-cache-disabling branch from 9c85bd3 to 79342eb Compare May 30, 2025 08:06
@kesselb
Copy link
Contributor

kesselb commented May 30, 2025

Another user reported a bug related to opcache.validate_timestamps = 0: nextcloud/server#52977.

I am uncertain about how to proceed with this pull request. My observation remains that many people skim through the page, copying suggestions without fully understanding the implications. Therefore, a default setting that works for most cases might be the better option here.

image

On the other hand, the page includes a danger block and two warning blocks and explains the entire topic thoroughly. So it is not that we are failing to raise awareness about it 🤷

@MichaIng
Copy link
Member

From reading the bug report, it actually really sounds like a bug. When updating entirely via web interface, Nextcloud should automatically invalidate relevant parts of the OPcache, isn't it?

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.

4 participants