Skip to content

Conversation

@michalsn
Copy link
Member

Description
This PR completes the implementation of the Superglobals class and migrates the entire codebase to use it consistently.

I started work on this about a month ago, and it turned out to be a tedious task. The scope of changes is relatively large, as this migration could not be done incrementally.

Since this class is internal and not intended for end-user consumption, no changelog entry has been added.

See: #7866

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@michalsn michalsn added enhancement PRs that improve existing functionalities refactor Pull requests that refactor code 4.7 labels Dec 28, 2025
Copy link
Contributor

@neznaika0 neznaika0 left a comment

Choose a reason for hiding this comment

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

I patched the latest commit 4.7 - there is an error.

1) CodeIgniter\Models\PaginateModelTest::testMultiplePager
Failed asserting that '<!-- DEBUG-VIEW START 1 SYSTEMPATH/Pager/Views/default_full.php -->\n
...
<!-- DEBUG-VIEW ENDED 1 SYSTEMPATH/Pager/Views/default_full.php -->\n
' [ASCII](length: 843) contains "?page_valid=1"" [ASCII](length: 14).

/development/tests/system/Models/PaginateModelTest.php:103

A lot of changes - I couldn't see them all at once.

UPD: Fixes #9392

@michalsn
Copy link
Member Author

@neznaika0 What do you mean by saying "I patched the latest commit 4.7"? Have you cloned this branch and run tests locally? This branch is working on the latest 4.7.

Anyway, I found the potential issue and hopefully fixed it. The strange thing is that it didn't pop up during tests.

@neznaika0
Copy link
Contributor

@michalsn I use few git features - it's easy to lose changes. I downloaded https://github.com/codeigniter4/CodeIgniter4/pull/9858.patch and I applied it on 4.7 at local.

@michalsn
Copy link
Member Author

@neznaika0 Ok, please verify if the issue still exists after recent changes - thanks.

@neznaika0
Copy link
Contributor

Good. Error is missing

*
* @param string $name The superglobal name (server, get, post, cookie, files, request)
*
* @return array<string, server_items>
Copy link
Member

Choose a reason for hiding this comment

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

This should not be just server_items, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

This includes all valid types at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

server_items covers all cases - that's why I applied it here.

michalsn and others added 4 commits January 2, 2026 13:10
Co-authored-by: John Paul E. Balandan, CPA <paulbalandan@gmail.com>
Co-authored-by: neznaika0 <ozornick.ks@gmail.com>
@michalsn
Copy link
Member Author

michalsn commented Jan 2, 2026

@paulbalandan @neznaika0 Thank you for all the suggestions - they have been applied.

Can someone confirm if running composer phpstan:baseline generates the ignore rules shown in GitHub Actions?
https://github.com/codeigniter4/CodeIgniter4/actions/runs/20657898023/job/59314243042?pr=9858

@paulbalandan
Copy link
Member

Yes, running the baseline removes the unmatched error.

@michalsn
Copy link
Member Author

michalsn commented Jan 2, 2026

Any advice? Every time I run composer phpstan:baseline, I keep getting additional ignore rules for is_cli().

I tried updating composer and removing build folder.

@paulbalandan
Copy link
Member

Can you try manually deleting the argument.count.neon then update the loader.neon to remove the entry?

Also, check if there's a local // @phpstan-ignore on that is_cli call. Remove those also.

@michalsn
Copy link
Member Author

michalsn commented Jan 2, 2026

No, manual // @phpstan-ignore in the file.

After manually deleting the argument.count.neon and updating the loader.neon, all came back after baseline generation.

@paulbalandan
Copy link
Member

I think this has something to do with the recent change in Boot where the is_cli mock is required. If the path is added to phpstan-bootstrap will it make any difference?

Copy link
Contributor

@neznaika0 neznaika0 left a comment

Choose a reason for hiding this comment

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

Good. Overall, I don't see any more problems.

May we note that unsetFiles($key), setFiles($key, $value), files($key) are not supported because the array does not change after creation?

michalsn and others added 2 commits January 2, 2026 17:08
Co-authored-by: neznaika0 <ozornick.ks@gmail.com>
@michalsn
Copy link
Member Author

michalsn commented Jan 2, 2026

May we note that unsetFiles($key), setFiles($key, $value), files($key) are not supported because the array does not change after creation?

@neznaika0 This is an internal class, so I'm not sure this needs to be documented. If you think it's useful, where would you suggest adding this note?

@michalsn
Copy link
Member Author

michalsn commented Jan 2, 2026

@paulbalandan Yes, this relates to the mocked function. Neither requiring Test/Mock/MockCommon.php in util_bootstrap.php nor adding it to Boot::bootConsole() resolves the issue. Seems like this happens only locally, and it prevents me from generating a proper baseline. Do you know of any caches I could clear that might affect this? I even tried changing the PHP version.

@paulbalandan
Copy link
Member

I cannot remember the actual command but there is one. You can check by vendor/bin/phpstan list

@michalsn
Copy link
Member Author

michalsn commented Jan 2, 2026

I see clear-result-cache, but it just deletes the cache directory - nothing I hadn't tried already. Maybe it's only happening on my side, but if more people run into this, we'll probably need to reconsider mocking is_cli() this way for now.

Copy link
Contributor

@neznaika0 neznaika0 left a comment

Choose a reason for hiding this comment

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

Thanks.

A note about Files can be added above the class, for example:

Actions with $_FILES are limited, since the data must be filled in once in the request.

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

Labels

4.7 enhancement PRs that improve existing functionalities refactor Pull requests that refactor code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants