Skip to content

Conversation

@sopex
Copy link
Contributor

@sopex sopex commented Sep 14, 2025

Creation of dynamic properties and the ${var} syntax have been deprecated since PHP 8.2

@fraenki fraenki self-assigned this Oct 1, 2025
@james8675309
Copy link

james8675309 commented Oct 27, 2025

I was going to put in an issue for this, but found this pull request.

It seems that if you have a version long enough and update enough times, a CRON job config may fail. This fix alleviates an error in SettingsController.php that actually prevents the settings UI elements from loading correctly and/or being saved if you change them... all because it couldn't call a warning that the CRON job was misconfigured. Below are how this affects the UI if you happen to have a CRON job misconfiguration

I already tested and confirmed that fixing this error corrects the UI failure.
opnsense-acme01
opnsense-acme02
opnsense-acme03
opnsense-acme04

Glad this will be fixed sooner rather than later! Cheers!

@sopex
Copy link
Contributor Author

sopex commented Oct 27, 2025

@james8675309 Thank you James!

@fraenki Any blockers here?

@fraenki
Copy link
Member

fraenki commented Nov 19, 2025

@sopex Aside from the ${var} syntax change, what problem are you trying to solve here?

@sopex
Copy link
Contributor Author

sopex commented Nov 19, 2025

@fraenki From what I remember, nothing.

I was trying to fix the depreciation error from showing up on the UI and the error log.
But apparently the "update schedule" fails to show up without fixing it, and enabling the plugin fails (at least for some users).

@fichtner
Copy link
Member

fichtner commented Nov 19, 2025

protected vars are not usable for deriving classes (locked in parent class scope) so if you use them you need to set them again which was done here correctly, but they are different copies. Maybe private would have been a better choice for these variables in LeCommon.

@sopex
Copy link
Contributor Author

sopex commented Nov 19, 2025

protected vars are not usable for deriving classes (locked in parent class scope) so if you use them you need to set them again which was done here correctly, but they are different copies. Maybe private would have been a better choice for these variables in LeCommon.

My understanding of PHP is that these will override the others, no?
Anyway, feel free to make any changes you deem necessary, but this must be merged. ACME is rather important and currently exibits very unstable behavior.

(Maybe you are correct, I haven't seen the code for 2+ months. In my mind, private would necessarily create a different copy)

@fichtner
Copy link
Member

fichtner commented Nov 19, 2025

My understanding of PHP is that these will override the others, no?

No, protected does not overlap across classes even if derived. It's actually "private" to each Class/File. ;)

currently exibits very unstable behavior

This may be due to deplyoment mode set to "development". I think these are all warnings only, no?

@sopex
Copy link
Contributor Author

sopex commented Nov 19, 2025

My understanding of PHP is that these will override the others, no?

No, protected does not overlap across classes even if derived. It's actually "private" to each Class/File. ;)

currently exibits very unstable behavior

This may be due to deplyoment mode set to "development". I think these are all warnings only, no?

Assuming @james8675309 was not in development mode (see above) these depreciation warnings, at least under specific circumstances, prevent the plugin from saving settings and activating properly.

On my limited testing what James reports above is at least partly observed on non development deployment mode, but I don't have a 100% vannila vm available for testing.

@fichtner
Copy link
Member

The API is more allergic to this it seems, but I haven’t seen this to be a widespread issue. Either way happy to move this forward with @fraenki’s approval

@fraenki
Copy link
Member

fraenki commented Nov 30, 2025

The PHP deprecation warnings were already addressed in another PR (#4824).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants