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

getSegment() fixed with conditions changed #3212

Closed
wants to merge 2 commits into from

Conversation

rajat315315
Copy link

Fixes #2962

Checklist:

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

@rajat315315 rajat315315 changed the title getSegments fixed with conditions changed getSegment() fixed with conditions changed Jul 3, 2020
@lonnieezell
Copy link
Member

Can you fix up the failing test please?

https://travis-ci.org/github/codeigniter4/CodeIgniter4/jobs/704736451

@rajat315315
Copy link
Author

rajat315315 commented Jul 4, 2020

Can somebody clarify the Unit test at:

public function testSegmentOutOfBound()
{
$this->pager->store('default', 10, 1, 10, 1000);
$this->assertEquals(1, $this->pager->getCurrentPage());
}

Is it correct?

I think L431 should be:

$this->assertEquals(10, $this->pager->getCurrentPage());

@michalsn
Copy link
Member

michalsn commented Jul 4, 2020

This would be a BC change. Surprisingly this is not covered by the URITest class.

Let's say we have this address: http://example.com/one/two. Now, calling $uri->getSegment(3) originally would return "" (empty string) but after this change, it will throw an exception.

In URI class we have a "rule" that restrains throwing exception until the segment isn't greater than n+1 (where n i actually defined segment of URI).

Anyway, I think that #2962 is pretty much covered by introducing setSilent() method. Other things can't be changed now without introducing BC changes.

@ivantcholakov
Copy link

ivantcholakov commented Jul 4, 2020

At the transition CI2 --> CI3 a convention was adopted. If a method does not "know" what to return, it returns NULL, not an empty string (edit: or FALSE sometimes). Is this valid for CI4?

@michalsn
Copy link
Member

michalsn commented Jul 4, 2020

@ivantcholakov No, CI4 has quite different rules - at least by default.

@paulbalandan
Copy link
Member

@michalsn are we still keeping this? or are we deferring this for a BC breaking version jump? If no, perhaps we can close this?

@michalsn
Copy link
Member

Personally, I think people rely too much on the current implementation to change it in 4.1. We can revisit this at some point but I'll close it for now.

@michalsn michalsn closed this Aug 29, 2020
@kenjis
Copy link
Member

kenjis commented Feb 14, 2023

See #7249

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.

Bug: URI class - working with segments is really strange
6 participants