feat: complete Superglobals implementation#9858
Conversation
There was a problem hiding this comment.
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 |
863789d to
539cb56
Compare
|
@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.
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?
@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 // edit |
neznaika0
left a comment
There was a problem hiding this comment.
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.
c46b15b to
feb717e
Compare
feb717e to
f9fdacc
Compare
Co-authored-by: John Paul E. Balandan, CPA <paulbalandan@gmail.com>
Co-authored-by: neznaika0 <ozornick.ks@gmail.com>
Co-authored-by: neznaika0 <ozornick.ks@gmail.com>
f9fdacc to
384c897
Compare
|
Thank you everyone, for reviews and code suggestions. |
|
Hmm, an important omission. If I need to create personal tests, then will the error need to be hidden? Let's say I'm testing Request() |
|
We have various ways to test the app: https://codeigniter.com/user_guide/testing/index.html |
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: