Skip to content
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

feat: make CLI::input() testable #6335

Merged
merged 6 commits into from
Aug 5, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Aug 3, 2022

Description
Fixes #6076

  • add PhpStreamWrapper
  • modify CLI::input() for testing

Checklist:

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

@kenjis kenjis added enhancement PRs that improve existing functionalities 4.3 labels Aug 3, 2022
@kenjis kenjis force-pushed the feat-CLI-input-testable branch from e33c5f9 to d787fe6 Compare August 3, 2022 03:12
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with PHP stream registration. I think @paulbalandan has done a lot of our work in this area and might be able to review. The test use looks good and run was successful so I assume this is working as intended. 💪

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

I only have two occasions where I encountered stream wrappers (infection/include-interceptor and dg/bypass-finals) which I think won't mess with this php wrapper since both are wrappers for the file protocol.

I just have a few comments.

system/CLI/CLI.php Show resolved Hide resolved
system/Test/PhpStreamWrapper.php Outdated Show resolved Hide resolved
@paulbalandan
Copy link
Member

Last thing, should this class also have a public $context property?

@kenjis
Copy link
Member Author

kenjis commented Aug 3, 2022

should this class also have a public $context property?

PHP manual says:

This property must be public so PHP can populate it with the actual context resource.
https://www.php.net/manual/en/class.streamwrapper.php#streamwrapper.props.context

So I'm not sure we really need it, but I added it.

@kenjis kenjis force-pushed the feat-CLI-input-testable branch from 6c24f32 to 3e2f780 Compare August 3, 2022 23:59
@kenjis
Copy link
Member Author

kenjis commented Aug 4, 2022

Added docs.

@kenjis kenjis force-pushed the feat-CLI-input-testable branch from 3e2f780 to c4cefdb Compare August 4, 2022 02:03
@kenjis kenjis force-pushed the feat-CLI-input-testable branch from dc9a285 to 2bf7753 Compare August 4, 2022 04:27
@MGatner
Copy link
Member

MGatner commented Aug 4, 2022

Thanks you two, good stuff!

@kenjis kenjis merged commit 6d81280 into codeigniter4:4.3 Aug 5, 2022
@kenjis kenjis deleted the feat-CLI-input-testable branch August 5, 2022 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants