Skip to content

fix path merge with path object #2245

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sparrow2009
Copy link

Mojo::Path's support of merging objects of class Mojo::Path (vs. strings) until now is implemented by object stringification followed by a reparsing using a default Mojo::Path object.

This approach works fine for merged objects with a character set of UTF-8 as this is also the default character set for Mojo::Path objects. It can fail however for other character sets.

Instead copy the path parts and the slash attributes from the source to the target object.

The missing tests for merging objects are included using the ISO-8859-15 character set. That way those tests can also function as regression tests. The ISO-8859-15 character set deviates from ISO-8859-1 and in turn from Unicode in that it reassigns code points of less common symbols (code point A4 which is the currency sign in ISO-8859-1/Unicode is replaced by the Euro currency sign e.g.).

Background:
Mojo::URL::to_abs() e.g. passes path objects to Mojo::Path::merge() in order to merge the path onto the base path today. And can fail in the way outlined above if the path object is not configured to be UTF-8.

Mojo::Path's support of merging objects of class Mojo::Path (vs. strings)
until now is implemented by object stringification followed by a reparsing
using a default Mojo::Path object.

This approach works fine for merged objects with a character set of UTF-8 as
this is also the default character set for Mojo::Path objects. It can fail
however for other character sets.

Instead copy the path parts and the slash attributes from the source to
the target object.

The missing tests for merging objects are included using the ISO-8859-15
character set. That way those tests can also function as regression tests.
The ISO-8859-15 character set deviates from ISO-8859-1 and in turn from
Unicode in that it reassigns code points of less common symbols (code
point A4 which is the currency sign in ISO-8859-1/Unicode is replaced
by the Euro currency sign e.g.).

Background:
Mojo::URL::to_abs() e.g. passes path objects to Mojo::Path::merge()
in order to merge the path onto the base path today. And can fail in the
way outlined above if the path object is not configured to be UTF-8.
@kraih kraih requested a review from Copilot June 16, 2025 16:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates Mojo::Path::merge to properly merge another Mojo::Path object by copying its parts and slash attributes instead of stringifying and reparsing, and adds tests (using ISO-8859-15) to cover this behavior.

  • Adjusted merge logic in lib/Mojo/Path.pm to detect and merge Mojo::Path objects directly
  • Added Merge path object subtest in t/mojo/path.t to validate correct structure and encoding handling

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
t/mojo/path.t Added use Mojo::Util qw(encode url_escape) and new subtest for merging paths with non-UTF-8 charset
lib/Mojo/Path.pm Enhanced merge to handle Mojo::Path objects without reparsing
Comments suppressed due to low confidence (1)

lib/Mojo/Path.pm:44

  • [nitpick] The indentation and brace placement in the new merge block differ from the surrounding code style. Align the new block with existing indentation patterns for better readability and consistency.
if (ref $path) {

@@ -41,11 +41,18 @@ sub merge {
my ($self, $path) = @_;

# Replace
return $self->parse($path) if $path =~ m!^/!;
if (ref $path) {
Copy link
Preview

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

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

In the object-handling branch, the final return uses the setter leading_slash, which may not explicitly return $self. To ensure merge remains chainable and returns the modified object, explicitly return $self; after updating parts and slash attributes.

Copilot uses AI. Check for mistakes.

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.

1 participant