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!

[toddr-bot]

PR Review — Request uri_base method have to return URI object in all cases

The refactoring is clean and the intent is correct — consolidating canonical into _common_uri and making uri_base return a URI object to match documented behavior and base. The code itself is sound, but this is a user-visible behavioral change (string → URI object) that needs accompanying tests, as the maintainers agreed in the discussion. No security or performance concerns. Merge after adding test coverage.

---

🟡 Important

1. uri_base return type change is a breaking behavioral change (lib/Dancer2/Core/Request.pm, L265-272)
Previously, uri_base returned a string because the =~ operator on $canon forced scalar/string context. Now it returns a URI object. While URI objects stringify via overloading, this is not fully transparent — code that does ref($uri_base), type checks, or uses string-only operations (e.g. Template Toolkit vmethods like [% uri_base.size %]) will break. The discussion thread reached consensus that this is the right fix since the docs say it should return a URI object, but tests covering the return type should accompany this change to document the new contract and catch regressions.

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

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

    return $uri;
}

2. Mutating the canonical URI from _common_uri before returning (lib/Dancer2/Core/Request.pm, L267-269)
_common_uri now returns $uri->canonical, and uri_base then mutates that object's path via $uri->path(''). This works because canonical returns a new URI object, so the mutation is safe. However, if _common_uri is ever changed to cache its result, this mutation would corrupt the cached object. Consider cloning ($uri->clone) before mutating, or adding a comment noting this dependency on canonical returning a fresh object.

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

3. No tests included for the behavioral change (lib/Dancer2/Core/Request.pm)
This PR changes the return type of uri_base from string to URI object and refactors _common_uri/base. Tests should verify: (1) base returns a URI object, (2) uri_base returns a URI object, (3) uri_base stringifies correctly with and without trailing slash, (4) both methods agree on the scheme/host/path components. The discussion thread explicitly called for tests to accompany this change.

---

Checklist

• No hardcoded secrets
• Return type consistency
• Test coverage for behavioral change — warning Dsl uri_for. #3

---

Summary

The refactoring is clean and the intent is correct — consolidating canonical into _common_uri and making uri_base return a URI object to match documented behavior and base. The code itself is sound, but this is a user-visible behavioral change (string → URI object) that needs accompanying tests, as the maintainers agreed in the discussion. No security or performance concerns. Merge after adding test coverage.

---

Automated review by Kōan

[toddr-bot]

@cromedome: @xsawyerx and I spent some time reviewing this today.

The code change itself is clean and correct. The consolidation of canonical into _common_uri eliminates duplication, and using $uri->path('') instead of regex substitution is more robust (avoids the original bug where =~ forced string context).

However, as discussed in this thread, this needs tests before merging. Specifically:

1. Verify uri_base returns a URI object (not a string) both with and without trailing / in SCRIPT_NAME.
2. Verify base and uri_base are consistent (both return URI objects, base includes trailing /, uri_base strips it).
3. Verify stringification produces the expected URLs in both cases.

This would codify the new contract and protect against future regressions.

[toddr-bot]

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

From a code review perspective, the change is correct and minimal. The =~ operator was indeed forcing $canon into string context, making the return type inconsistent depending on whether the path was / or not. The fix properly uses $uri->path('') to strip the trailing slash while preserving the URI object type.

Recommendation: merge this with added tests, and note the behavioral change in migration/release docs as @veryrusty suggested. The documentation already promises a URI object return, so this aligns behavior with docs rather than introducing new behavior.