Improvements for getSegment() method from URI class

[michalsn]

Description
This PR introduces some improvements for URI::getSegment() method, adding two optional parameters:

• $default - default value to use if given segment is empty
• $silent - if set to true it prevents throwing an exception

// this will return 'foo'
$uri->getSegment(11, 'foo', true);
// and this will throw an exception - as usual
$uri->getSegment(11);

This is related to the issue #2949

I think that this might be very handy in some situations. Handling the scenario where we want to check if given parameter is out there is not very friendly for developers right now. These changes will save some of us a lot of code.

Checklist:

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

[jreklund]

Don't validate anything:

testSegmentOutOfRangeWithDefaultValue

How can this even be true? You don't suppress the message, so it should throw an error.

$this->assertEquals('script', $uri->getSegment(3, 'script'));
$this->assertEquals('', $uri->getSegment(3));

You don't need the silent part. If it existed, it would be populated or give you the default value.

public function getSegment(int $number, string $default = ''): string
{
	// The segment should treat the array as 1-based for the user
	// but we still have to deal with a zero-based array.
	$number -= 1;

	if ($default !== '' && $number > count($this->segments))
	{
		throw HTTPException::forURISegmentOutOfRange($number);
	}

	return $this->segments[$number] ?? $default;
}

Personally I would love to see this instead, but that means it gives you null in some cases. (Haven't tested it, if there are cases where it dosen't exist, and will give you null.) And that will breaks things I guess. :-(

public function getSegment(int $number, string $default = null): ?string
{
	// The segment should treat the array as 1-based for the user
	// but we still have to deal with a zero-based array.
	$number -= 1;

	if (! is_null($default) && $number > count($this->segments))
	{
		throw HTTPException::forURISegmentOutOfRange($number);
	}

	return $this->segments[$number] ?? $default;
}

[michalsn]

Don't validate anything:

testSegmentOutOfRangeWithDefaultValue

How can this even be true? You don't suppress the message, so it should throw an error.

$this->assertEquals('script', $uri->getSegment(3, 'script'));
$this->assertEquals('', $uri->getSegment(3));

Well... you have to look at the code and check how it is working now. The Exception is thrown only if you're trying to get a segment that is greater than total segments + 2. So this line of code $this->assertEquals('', $uri->getSegment(3)) would be perfectly valid even without merging my changes.

About the part that proposes to skip the $silent parameter.

This implementation below wouldn't allow us to return an empty string for segments that don't exist - rather unacceptable.

public function getSegment(int $number, string $default = ''): string
{
	// The segment should treat the array as 1-based for the user
	// but we still have to deal with a zero-based array.
	$number -= 1;

	if ($default !== '' && $number > count($this->segments))
	{
		throw HTTPException::forURISegmentOutOfRange($number);
	}

	return $this->segments[$number] ?? $default;
}

And this one, as you pointed out, would return null in some cases even when we don't really want it.

public function getSegment(int $number, string $default = null): ?string
{
	// The segment should treat the array as 1-based for the user
	// but we still have to deal with a zero-based array.
	$number -= 1;

	if (! is_null($default) && $number > count($this->segments))
	{
		throw HTTPException::forURISegmentOutOfRange($number);
	}

	return $this->segments[$number] ?? $default;
}

So, that's why the $silent parameter is required. Proposed implementation changes nothing in the way this method work when you don't use these new parameters. I think this is a reasonable compromise to simplify the work of developers and don't introduce BC changes.

[jreklund]

Yeah, sorry about that. That's even more stupid in my opinion. Why would you allow fetching something that's not there. So I didn't think about +1 thing.

setSegment() works the same now that I look at it. You can set it +2 more than there are segments. It still gets appended as +1.

Did some actual testing with this one now, instead of just writing from memory. I can't get any segment with '', unless I have manually used setSegment(). Are there any special cases I'm missing? As I can't trigger this one.

isset($this->segments[$number]) && $this->segments[$number] !== ''

I get the same result with the one that we had before.

$this->segments[$number] ?? $default

Too bad we have made a real release, because "fixing" the problem will require all programmers to use three parameters. As we can't do BC changes. Personally I get the same result with this one. Less code yay!

public function getSegment(int $number, ?string $default = '', bool $silent = false): ?string
{
	// The segment should treat the array as 1-based for the user
	// but we still have to deal with a zero-based array.
	$number -= 1;

	if (! $silent && $number > count($this->segments))
	{
		throw HTTPException::forURISegmentOutOfRange($number);
	}

	return $this->segments[$number] ?? $default;
}

Seeing how this works, I'm just going to replace it, if I will ever need to use it. I have already replaced some, as they differ to much from they "should" work, based on how other frameworks have solved the same thing.

public function getSegment(int $number, string $default = null): ?string
{
    return $this->segments[$number - 1] ?? $default;
}

[michalsn]

The thing with setSegment() is actually reasonable and I understand it since you can't have segments that don't exist, like ie. there are no segments 4 and 5 but then all of sudden you have segment 6? That would have no sense and would be not valid.

Did some actual testing with this one now, instead of just writing from memory. I can't get any segment with '', unless I have manually used setSegment(). Are there any special cases I'm missing? As I can't trigger this one.

isset($this->segments[$number]) && $this->segments[$number] !== ''

I get the same result with the one that we had before.

$this->segments[$number] ?? $default

Yeah... you are right. I have no idea why I changed it - I will send an update. Thanks.

Seeing how this works, I'm just going to replace it, if I will ever need to use it. I have already replaced some, as they differ to much from they "should" work, based on how other frameworks have solved the same thing.

I guess every framework has its own "things". And "this thing" isn't something really terrible - as long as this change will be merged ;P

We all have some habits and expectations about how something "should" works. And the truth is that before the v4 release there were not so many real-world applications built, so there are probably still a few things to polish up.

[jreklund]

If you only want valid positions with setSegment(), you should not be able to set 5 in case there aren't one in segment 4. Don't see the use of jumping one over, same with getSegment(). This may just be me thought.

How's your take on adding a setSilent() like the MigrationRunner? So you can permanently disable the throws as well (need to silently apply to all functions in URI). Because always setting three parameters to getSegment() will get tiresome.

Of course it will be differences, but CI4 are the only major player that throws an error message for this. Sure you can set a standard.

// You are more likely to use an if statement than use it an try block.
if(($id = $this->request->uri->getSegment(3)) === null)
{
    throw new Exception('No id found...');
}

// Instead of 
try
{
    $id = $this->request->uri->getSegment(3);
}
catch(Exception $e)
{
   throw new Exception('No id found...');
}

// Or this to be able to use it in an if statement later.
try
{
    $id = $this->request->uri->getSegment(3);
}
catch(Exception $e)
{
   $id = null
}

if($id === null)
{
    // Load error to user
}
else
{
   // Load real content
}

[michalsn]

If you only want valid positions with setSegment(), you should not be able to set 5 in case there aren't one in segment 4.

I haven't looked at setSegment() before but now I checked it and you're right. I would call it a bug. I think I will open an issue to address these problems with getSegment() and setSegment() because besides the proposal from this PR, there is a bigger problem in my opinion.

Don't see the use of jumping one over, same with getSegment(). This may just be me thought.

In some applications, I used it heavily - ie. when we had many categories with different options. We didn't always have a full URL, so support for this and default values were a real saver there.

setSilent() - at first I was like "no"... but actually it makes sense. But I guess this would have to be addressed in a different PR. For now, I will probably wait for the opinion of someone from the core team regarding all of this.

Personally, I would prefer one-liner with three parameters than catching the exception every single time. The other advantage is that you can set a default value really easily.

[MGatner]

I was thinking of a setSilent() method before I even got that far in your conversation. I like that it is consistent with other components and that it keeps parameters to a minimum. This PR would be adding two parameters to one method, but if we want a silent option down the road it would add more parameters to another method. I'm open to counter-arguments but I'd rather see this resolved now before we create the new public method and would need a BC to adjust it later.

[MGatner]

Should have also specified that I think this is a good change and a very nice PR! Thanks for the work, and for the review from @jreklund

[jreklund]

I think after the setSilent() method we could implement a default value as well (but without $silent of course), as it's a nice feature. :-)