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

fix: Path checking for the existence of a namespaced helper file #9350

Closed
wants to merge 1 commit into from

Conversation

yllumi
Copy link

@yllumi yllumi commented Dec 30, 2024

Description
The update in version 4.5.6 caused an error in the logic check line for the path of the namespaced helper file. The return value of $loader->locateFile is a string containing the file path if found, and false if not found. The FileNotFoundException should be executed if $path value is false.

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 the tests needed Pull requests that need tests label Dec 30, 2024
@neznaika0
Copy link
Contributor

@kenjis sorry, i missed it in
#9249 . There was a replacement:

- if (empty($path)) {
+ if ($path !== '') {

@@ -598,7 +598,7 @@ function helper($filenames): void
if (str_contains($filename, '\\')) {
$path = $loader->locateFile($filename, 'Helpers');

if ($path !== '') {
if (!$path) {
Copy link
Member

Choose a reason for hiding this comment

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

should be strict empty string or false

Suggested change
if (!$path) {
if ($path === false || $path === '') {

Copy link
Member

Choose a reason for hiding this comment

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

Ok, $path === false should be enough, since empty string should never happen

Suggested change
if (!$path) {
if ($path === false) {

@samsonasik
Copy link
Member

@yllumi it is recommended to create a branch per PR, you can read guideline for it

https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#how-to-guide

Also, please provide failing test case on tests directory for the patch, thank you.

@samsonasik
Copy link
Member

@yllumi I created new PR with include test case for it:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests needed Pull requests that need tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants