View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 5594 | Composr | core | public | 2024-02-07 01:28 | 2024-11-28 15:01 |
| Reporter | PDStig | Assigned To | PDStig | ||
| Priority | normal | Severity | feature | ||
| Status | resolved | Resolution | fixed | ||
| Summary | 5594: Consider declare(strict_types=1) in codebase (rolling) | ||||
| Description | Since it is not possible at this time to maintain the special ocp PHP (much less compile it on certain machines), we should consider adding declare(strict_types=1) at the top of every PHP file in the v11 codebase. Be cautious about its implementation; it must also be explicitly included in eval code (unless we're pulling code from a file that already has it declared). | ||||
| Tags | Roadmap: Over the horizon, Roadmap: v11 partial implementation, Type: Standards compliance | ||||
| Attach Tags | |||||
| Time estimation (hours) | |||||
| Sponsorship open | |||||
|
|
Bumped to v12 / rolling issue. This will require a major refactoring of the code because of instances like this one for example: if (@strlen(cms_file_get_contents_safe($_SERVER['SCRIPT_NAME'], FILE_READ_LOCK)) > 4500) { Even though strlen is suppressed, this still throws a TypeError when cms_file_get_contents_safe evaluates to false. Type checking must happen before the strlen call. There are many cases of things like this throughout the code. So a better approach is to implement this gradually. |
|
|
I'm not sure this is a good idea. One of the goals with v11 was to make production deployments less fragile. We have the new config options for toning down the handling of PHP Notices, Warnings, etc. I don't know if strict_types would be going against this - or would we also still be able to filter them out based on configuration just like we do PHP Notices? I thought type errors were fatals when this was on. The fact @ doesn't stop it suggests it is fatal. If we can no longer maintain/deploy ocProducts PHP, I think it makes more sense to reassess what we were trying to achieve with it and how important that is given new realities. |
|
|
Yeah I have been giving this thought as well. That's one of the reasons I quickly decided against considering it for v11 at least. You are right. TypeErrors are fatal. They can, however, be handled, at least I assume they can because Composr's critical error handling handled them when I gave it a try. PHP's implementation of strict_types is not very intuitive. It's runtime-level. It has to be defined on every file you want it to run. It must also be defined in eval code if you want eval code to be strict type checked. It must be defined top-level. You can't dynamically turn it off/on. Etc. This means we can't simply implement it just for DEV_MODE. "If we can no longer maintain/deploy ocProducts PHP, I think it makes more sense to reassess what we were trying to achieve with it and how important that is given new realities." I still think XSS checking and type strict checking is very important and should still be strong considerations. XSS attacks are on the rise, I believe, in general (especially with WordPress). But you're right. We can't maintain / deploy it anymore. |
|
|
ocProducts PHP predates PHP itself supporting strict typing, and I retroactively added forcing the strict_types option on as an addon to what I had previously done. The PHP community had an extremely rough time agreeing on strict typing. The Zend developers, who were stewarding the project at the time, were vehemently against it at all. I would have preferred an ini setting, but the issue there is that it forces all third party code to support strict types too - which many PHP core devs could never get behind as it was such a fundamental changing in the expectations for PHP developers and compatiblity. The XSS issue is almost entirely mitigated by CSP, which did not exist when I implemented the XSS functionality for ocProducts PHP. |
|
|
I agree. I'm closing this issue under the following premise: Strict type checking should mainly be done at the development / IDE level, not runtime, as runtime checks make for very fragile code (since Type Errors are fatal). We have CQC which helps with this. And some IDEs can check for it as well, although ultimately only CQC is able to type-check Composr types. I think that's good enough though couple with PHP's type support as of 7 and 8. |
|
|
I'm reopening this, because in dev-mode we could inject it when code files are loaded via sources/global.php. |
|
|
What to do about ocProducts PHP is a deeper question worth considering. It makes PHP stricter than any other PHP tool out there can do, and we have coded to that level of strictness. One advantage to that is it will make Composr more easily portable to a language other-than-PHP if we so wish, e.g. Go or Rust. I think for now it would be fine to not force Composr devs to run ocProducts PHP, would be good to add publicly-supported strictness in dynamically, but also not erase the possibility of using ocProducts PHP as a bridge out of PHP in the future if we consider it appropriate. |
|
|
I started working on this. I modified the Composr system to use bootstrap.php instead of global.php as the initial entry point. bootstrap.php contains the bare minimal code necessary to load up global.php or minikernel.php with strict_types if in DEV_MODE. All entry-point scripts have been modified to use bootstrap.php instead of global.php and then call up global from there. This ensures we can get strict type checks on global / minikernel as well. So far so good; the code is working as expected, and I'm getting a lot of strict type errors that need fixed (e.g. passing in false when we shouldn't, floats where ints should go, and so on). This issue will take a while to complete as I essentially need to spider through the sitemap to find any type errors we missed all these months not using ocp PHP. But that's fine; the good news is we have strict type checking again despite not having ocp PHP. |
|
|
Marking resolved. I implemented it. There may still be some bugs because essentially, I had to also incorporate what was once planned for v12 (5473) in this issue (otherwise stack traces in dev mode are completely borked). I will also be making a post there because I decided not to implement it as the original issue indicated. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2024-02-07 01:28 | PDStig | New Issue | |
| 2024-02-07 01:28 | PDStig | Status | Not Assigned => Assigned |
| 2024-02-07 01:28 | PDStig | Assigned To | => user4172 |
| 2024-02-07 01:28 | PDStig | Tag Attached: Roadmap: v11 | |
| 2024-02-07 01:54 | PDStig | Tag Detached: Roadmap: v11 | |
| 2024-02-07 01:55 | PDStig | Tag Attached: Roadmap: v12 | |
| 2024-02-07 01:56 | PDStig | Note Added: 0008292 | |
| 2024-02-07 01:56 | PDStig | Tag Attached: Roadmap: v11 partial implementation | |
| 2024-02-07 01:57 | PDStig | Note Edited: 0008292 | |
| 2024-02-07 01:57 | PDStig | Summary | Consider declare(strict_types=1) in codebase => Consider declare(strict_types=1) in codebase (rolling) |
| 2024-02-10 19:53 | Chris Graham | Note Added: 0008309 | |
| 2024-02-10 20:21 | PDStig | Note Added: 0008314 | |
| 2024-02-10 20:44 | Chris Graham | Note Added: 0008319 | |
| 2024-02-11 04:34 | PDStig | Status | Assigned => Closed |
| 2024-02-11 04:34 | PDStig | Resolution | open => won't fix |
| 2024-02-11 04:34 | PDStig | Note Added: 0008333 | |
| 2024-02-11 04:35 | PDStig | Note Edited: 0008333 | |
| 2024-03-26 00:58 | PDStig | Tag Renamed | Roadmap: v12 => Roadmap: Over the horizon |
| 2024-07-25 19:27 | Chris Graham | Status | Closed => Not Assigned |
| 2024-07-25 19:27 | Chris Graham | Resolution | won't fix => reopened |
| 2024-07-25 19:28 | Chris Graham | Note Added: 0008944 | |
| 2024-07-25 19:31 | Chris Graham | Note Added: 0008945 | |
| 2024-07-25 19:31 | Chris Graham | Category | General / Uncategorised => core |
| 2024-07-25 19:32 | Chris Graham | Tag Attached: Type: Standards compliance | |
| 2024-11-24 22:22 | PDStig | Note Added: 0009681 | |
| 2024-11-28 15:01 | PDStig | Status | Not Assigned => Resolved |
| 2024-11-28 15:01 | PDStig | Resolution | reopened => fixed |
| 2024-11-28 15:01 | PDStig | Note Added: 0009688 | |
| 2024-11-28 15:01 | PDStig | Relationship added | related to 5473 |
