Request uri_base method have to return URI object in all cases

[bor]

The PR makes consistency better in cases of: - with the 'base' method, now both return URI objects - in various cases of '/' at end

The issue was that =~ converts the value into a scalar in case of '/' at the end.

[veryrusty]

Thanks for the PR @bor.

I concur it would have been better if request->base and request->uri_base were consistent and returned URI objects, which would then match the behaviour of what Plack::Request does for its url related methods.

The refactor of _common_uri always canonicalises the url is good.

@SysPete @cromedome The existing behaviour (and code) of string return from uri_base looks to have been taken directly from Dancer1. Our docs for uri_base include examples for url construction in templates. I'm concerned that breakage may occur if we change to a URI object at this point. eg. Tempalte::Toolkit vmethods, where `[% uri_base.size %] would become a method call on the URI object.

Should we consider another method that returns request uri_base as a URI object ? Something like the following would suffice.

sub base_uri {
     my $self = shift;
     my $uri  = $self->_common_uri;

     if ( $uri->path eq '/' ) {
         $uri->path('');
     }

     return $uri;
 }
 
 sub uri_base { shift->base_uri->as_string }

[cromedome]

I share @veryrusty's concern here. I am all for consistency... but not so much for breakage. Any suggestions for a method name? base_uri vs uri_base leaves me very confused :)

[bor]

I've checked the documentation and didn't found any base_uri. entries (call it as method).

At the moment base_uri method may returns both object or string in a case of / at the end. So IMHO it is in breakable stage already. (I've encountered that)

The documentation says Same thing as C<base> above. And for base we see: Returns a L<URI> object (which stringifies to the URL, as you'd expect). So it looks like base_uri should return object. And that's why I've called that as a "fix" ;)

[SysPete]

Since current behaviour is clearly broken I'd like to propose that we mark uri_base as deprecated and remove it from all docs. The new method as proposed by @veryrusty is fine for me with the name proposed as replacement.

[veryrusty]

Yeah, two similarly named methods sucketh (who suggested that?!).

The uri_base method was copied from Dancer1 and currently behaves the same there. If we were deprecate it we need to note that in the migration docs from D1. If we apply the change in this PR we need to note that the migration docs that it has changed behaviour. A bit of damned if we do and damned if we don't.

After reflection, the fix in this PR is the way to go. However we need tests and documentation, and possibly a blog post somewhere about it.

[cromedome]

I can deal with the docs and blog post as part of the release if someone has time to cobble some tests together.

I'll create an issue for creating a real deprecation policy, but for now, maybe go with two minor versions out and throw a warning, ie. "Method marked as deprecated and will be removed in Dancer2 0.500000". Thoughts?

[cromedome]

@xsawyerx and I spent some time reviewing this today.

Our documentation says we return an object, but as @bor points out, we don't always. We're thinking we should approve and merge this, write the appropriate docs and blog post, and deal with the fallout.

@veryrusty and @SysPete - penny for your thoughts?

[cromedome]

@veryrusty and @SysPete - penny for your thoughts? Thank you!