Fix PHP 8 number formatting compatibility#238
Open
somethingwithproof wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the plugin’s number formatting to avoid PHP 8 strlen()/substr() type errors by normalizing the rounded value to a string before applying string functions. It also introduces wide-ranging ancillary changes (strict-types declarations across many plugin files plus a substantial new test/tooling and security regression suite), which materially broadens the scope beyond the stated “PHP 8 number formatting compatibility” fix.
Changes:
- Fix
format_number()PHP 8 compatibility by ensuring the rounded value is a string before calling string functions, while preserving integer vs decimal output behavior. - Add
declare(strict_types=1);across many PHP entrypoints and library files. - Add a large Pest-based regression/security test suite plus supporting tooling/config (Composer dev dependency, PHPUnit/PHPStan/Mutation testing configs) and new security documentation.
Reviewed changes
Copilot reviewed 88 out of 89 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| weathermap-cacti-rebuild.php | Adds strict_types declaration. |
| weathermap-cacti-plugin.php | Adds strict_types declaration. |
| weathermap-cacti-plugin-mgmt.php | Adds strict_types declaration. |
| weathermap-cacti-plugin-editor.php | Adds strict_types declaration. |
| lib/WeatherMap.functions.php | Adds strict_types; fixes format_number() PHP 8 string/length handling. |
| lib/WeatherMap.class.php | Adds strict_types declaration. |
| lib/WMVector.class.php | Adds strict_types declaration. |
| lib/WMPoint.class.php | Adds strict_types declaration. |
| lib/WMLine.class.php | Adds strict_types declaration. |
| lib/WeatherMapNode.class.php | Adds strict_types declaration. |
| lib/WeatherMapLink.class.php | Adds strict_types declaration. |
| lib/WeatherMap.keywords.inc.php | Adds strict_types declaration. |
| lib/HTML_ImageMap.class.php | Adds strict_types declaration. |
| lib/geometry.php | Adds strict_types declaration. |
| lib/poller-common.php | Adds strict_types declaration. |
| lib/ds-common.php | Adds strict_types declaration. |
| lib/editor.inc.php | Adds strict_types declaration. |
| lib/editor.actions.php | Adds strict_types declaration. |
| lib/index.php | Adds strict_types declaration. |
| lib/pre/index.php | Adds strict_types declaration. |
| lib/pre/WeatherMapPreProcessorExample.php | Adds strict_types declaration. |
| lib/post/index.php | Adds strict_types declaration. |
| lib/post/WeatherMapPostProcessorExample.php | Adds strict_types declaration. |
| lib/datasources/index.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_cacti.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_cactihost.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_cactithold.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_dsstats.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_fping.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_mrtg.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_rrd.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_snmp.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_snmp2c.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_snmp3.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_static.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_tabfile.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_time.php | Adds strict_types declaration. |
| lib/datasources/WeatherMapDataSource_wmdata.php | Adds strict_types declaration. |
| setup.php | Adds strict_types declaration. |
| index.php | Adds strict_types declaration. |
| check.php | Adds strict_types declaration. |
| cli/index.php | Adds strict_types declaration. |
| cli/bristle.php | Adds strict_types declaration. |
| cli/cacti-integrate.php | Adds strict_types declaration. |
| cli/cacti-mapper.php | Adds strict_types declaration. |
| cli/convert-to-dsstats.php | Adds strict_types declaration. |
| cli/map-tidyup.php | Adds strict_types declaration. |
| cli/upgrade_configs.php | Adds strict_types declaration. |
| docs/index.php | Adds strict_types declaration. |
| configs/index.php | Adds strict_types declaration. |
| output/index.php | Adds strict_types declaration. |
| images/index.php | Adds strict_types declaration. |
| images/backgrounds/index.php | Adds strict_types declaration. |
| images/icons/index.php | Adds strict_types declaration. |
| images/objects/index.php | Adds strict_types declaration. |
| locales/index.php | Adds strict_types declaration. |
| locales/LC_MESSAGES/index.php | Adds strict_types declaration. |
| locales/po/index.php | Adds strict_types declaration. |
| .gitignore | Ignores .omc/. |
| composer.json | Adds Pest as a dev dependency and dev autoload bootstrap. |
| phpunit.xml | Adds PHPUnit configuration (used by Pest) and suppresses deprecations. |
| phpstan.neon | Adds PHPStan configuration (with PHP version target). |
| infection.json | Adds Infection mutation testing configuration. |
| tests/Pest.php | Adds Pest entry file requiring the test bootstrap. |
| tests/bootstrap.php | Adds lightweight test bootstrap with Cacti function stubs. |
| tests/Helpers/GlobalStubs.php | Adds minimal placeholder for future test bootstrapping. |
| tests/Unit/FormatNumberTest.php | Adds regression coverage for PHP 8 decimal formatting behavior. |
| tests/Unit/RenderColourTest.php | Adds unit coverage around render_colour() sentinel behavior. |
| tests/Unit/BandwidthPercentageTest.php | Adds unit coverage for bandwidth percentage zero-guard behavior. |
| tests/Unit/CronFieldOrderTest.php | Adds unit coverage for cron-field destructuring order. |
| tests/Unit/EditorActionNormalizationTest.php | Adds unit checks for editor action sanitization wiring. |
| tests/Integration/SettingsPersistenceTest.php | Adds integration-style source assertions around settings persistence SQL usage. |
| tests/Integration/EditorActionWiringTest.php | Adds integration-style source assertion for editor action dispatch. |
| tests/Handoff/XssEscapingHandoffTest.php | Adds handoff checks around HTML escaping at output boundaries. |
| tests/Handoff/DataSourceContractTest.php | Adds handoff checks for datasource ReadData() return contract. |
| tests/Smoke/PluginFilesSyntaxTest.php | Adds token-based parse/syntax smoke tests plus basic plugin structure checks. |
| tests/Mutation/FixedBugRegressionTest.php | Adds mutation/regression tests for multiple historical bugs and confirms fixes in source. |
| tests/Security/SetupStructureTest.php | Adds security-oriented checks for setup structure. |
| tests/Security/RedirectSafetyTest.php | Adds checks for header('Location...') followed by exit/die/return. |
| tests/Security/PreparedStatementConsistencyTest.php | Adds checks for prepared statement usage and SQL placeholder consistency. |
| tests/Security/AuthGuardTest.php | Adds checks for auth guard presence and numeric ID validation patterns. |
| tests/Security/OutputEscapingTest.php | Adds checks for escaping usage patterns in UI-related files. |
| tests/Security/CommandInjectionTest.php | Adds command injection surface tests/documentation cases. |
| tests/Security/Php74CompatibilityTest.php | Adds source-level checks intended to guard against PHP 8-only APIs in production code. |
| tests/Security/PathTraversalTest.php | Adds path traversal surface tests and documents expected sanitization behavior. |
| tests/E2E/EditorActionNoRawDispatchRegressionTest.php | Adds a source-level regression check around editor action dispatch. |
| SECURITY.md | Adds a security policy and reporting guidance. |
| SECURITY-AUDIT.md | Adds an audit report documenting findings and remediation notes. |
| BACKLOG.md | Adds an issue backlog derived from the security audit. |
Comment on lines
+1
to
+7
| { | ||
| "name": "cacti/plugin_weathermap", | ||
| "description": "plugin_weathermap plugin for Cacti", | ||
| "license": "GPL-2.0-or-later", | ||
| "require-dev": { | ||
| "pestphp/pest": "^1.23" | ||
| }, |
| * 3. Must not contain '\' (Windows path traversal defense-in-depth — added fix) | ||
| * 4. Passes through wm_editor_sanitize_uri (strips dangerous URI chars) | ||
| * | ||
| * PHP 8.4 is required, so null-byte injection risk (PHP < 8.0) is absent. |
| it('does not leave raw action assignment in the editor entrypoint', function (): void { | ||
| $entry = file_get_contents(dirname(__DIR__, 2) . '/weathermap-cacti-plugin-editor.php'); | ||
|
|
||
| expect($entry)->not->toContain("if (isset_request_var('action')) {\n\t\$action = get_nfilter_request_var('action');\n}"); |
Comment on lines
+6
to
+10
| level: 6 | ||
| paths: | ||
| - src | ||
| - tests | ||
| phpVersion: 80400 |
b754901 to
504d75e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
git diff --checkThis is separate from #225 so the compatibility fix can be reviewed and merged independently.