Skip to content

Conversation

@paulbalandan
Copy link
Member

Description
I've noticed that getSegment's code deducts 1 from the index parameter before checking which made me realize that it is one-based instead of zero-based. This PR updates GeneratorCommand::getClassName() to use that.

In retrospect, why was it that way?

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@paulbalandan
Copy link
Member Author

I think this should be getSegment(2).

@paulbalandan paulbalandan changed the title Use CLI::getSegment(1) to refer to the first argument Use CLI::getSegment(2) to refer to the first argument of generator command Sep 3, 2020
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

Just waiting for tests to pass before merging.

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.

Could we get a test case showing what this fixes? How has this been working so far?

@paulbalandan
Copy link
Member Author

Could we get a test case showing what this fixes? How has this been working so far?

Unfortunately, I think we cannot setup an automated test for this. The concrete classes extending GeneratorCommand all require class names as argument, so the empty string given here will trigger a CLI::prompt which is currently untestable. You may test them manually.

Analyzing how CLI parses the arguments and stores them, here is an illustration:

php spark make:controller blog -bare

CLI::getSegment(0) =>> undefined // since we are accessing the -1 index
CLI::getSegment(1) =>> 'make:command' // first segment after the name of invoking program is the name of the command
CLI::getSegment(2) =>> 'blog' // next segments would be the arguments passed to the command
CLI::getOption('bare') =>> true // since it is set as a flag

@MGatner
Copy link
Member

MGatner commented Sep 3, 2020

All good! I trust your assessment.

@MGatner MGatner merged commit 4fc3088 into codeigniter4:develop Sep 3, 2020
@paulbalandan paulbalandan deleted the cli-getsegment branch September 3, 2020 14:11
@John-Betong
Copy link

John-Betong commented Sep 11, 2020

I have just been working on my dynamic menu system and had to count($uri->getSegments()) to prevent the following error being thrown:

// $tmpSub = $uri->getSegment(2) // MAYBE THROW ERROR

Request URI segment is out of range:

Online Demo using $uri->getSegment(2) :

https://ci4-strict.tk/info

@michalsn
Copy link
Member

@John-Betong
Copy link

@michalsn,

@John-Betong You can use setSilent() instead: https://codeigniter4.github.io/userguide/libraries/uri.html#uri-segments

That's new to me and could come in handy some time in the future.

I don' think I like hiding exceptions, far prefer to know what is happening.

I prefer testing for results and using if/else/endif statements.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants