-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: complete Superglobals implementation
#9858
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
base: 4.7
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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:103A lot of changes - I couldn't see them all at once.
UPD: Fixes #9392
|
@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 Anyway, I found the potential issue and hopefully fixed it. The strange thing is that it didn't pop up during tests. |
|
@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. |
|
@neznaika0 Ok, please verify if the issue still exists after recent changes - thanks. |
|
Good. Error is missing |
| * | ||
| * @param string $name The superglobal name (server, get, post, cookie, files, request) | ||
| * | ||
| * @return array<string, server_items> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
863789d to
539cb56
Compare
Co-authored-by: John Paul E. Balandan, CPA <paulbalandan@gmail.com>
Co-authored-by: neznaika0 <ozornick.ks@gmail.com>
|
@paulbalandan @neznaika0 Thank you for all the suggestions - they have been applied. Can someone confirm if running |
|
Yes, running the baseline removes the unmatched error. |
|
Any advice? Every time I run I tried updating composer and removing |
|
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 |
|
No, manual After manually deleting the |
|
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? |
neznaika0
left a comment
There was a problem hiding this 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?
Co-authored-by: neznaika0 <ozornick.ks@gmail.com>
@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? |
|
@paulbalandan Yes, this relates to the mocked function. Neither requiring |
|
I cannot remember the actual command but there is one. You can check by |
|
I see |
neznaika0
left a comment
There was a problem hiding this 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.
Description
This PR completes the implementation of the
Superglobalsclass 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: