Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

There is a slight chance to introduce a break, but only if the path further altered after it changed to the relative. One possible mitigation would be to checking the input; it is a file or directory

I think the check is worth it. is_dir is cached, so it won't slow down things much.

I think the check is worth it. is_dir is cached, so it won't slow down things much.

You mean we will be checking the directory and adding a slash regardless of the input string?

I am assuming the method will still have output when a path not exists, then it can produce different output on subsequent run or different state of the filesystem:

$filesystem =new Symfony\Component\Filesystem\Filesystem();
$relative = $filesystem->makePathRelative('/path/to/project/resources', '/path/to/project');

if ($filesystem->exists('/path/to/project/resources')) {
    mkdir('/path/to/project/resources' );
}
echo $relative . PHP_EOL;

// -> '/path/to/project/resources'  if dir not exist
// -> '/path/to/project/resources/' if dir exist (or second run)

I believe it will create more problems than we currently have.
So I think changing the output is a better way forward.

I'm closing here because I think this can create troubles in existing codebases.
I agree it could have been a better, but the current behavior is too old to break.
Also, there are ways around (eg using the method on dirname($file))
Thanks for submitting.

I understand the reasons.

Given the $relativePath is the output of the makePathRelative() method, and $endPath is the first argument, the following snippet removes the trailing slash if not present in the input:

if (!str_ends_with($endPath, '/')) {
    $relativePath =  rtrim($relativePath, '/');
}

This needs to be definitely fixed, when a non-dir path is given (path not ending with /), the result must never have / appended. I agree this can imply BC break, bug the current impl. is simply wrong. Please reopen this PR or let me know if I or the original author should submit it newly.

+1, I use this function to make a path to a stylesheet file (e.g. /srv/www/[...]/folder/app.css relative to another folder,
currently makePathRelative returns the path with a slash at the end (folder/app.css/).

@nicolas-grekas Sensible fix like https://github.com/atk4/ui/blob/8f7a986c629a0adf3342c6d285bbef613fe52095/src/App.php#L655-L658 works /w current/buggy Symfony and /w this PR/fixed Symfony so fixing it should be fine as long as the developer was aware of this issue and counted with the possibility to be fixed in the future.

For me, it is clearly a bug in Symfony and it should be fixed sooner or later.

Can you please reconsider this PR, if not for 4.4, can it be integrated into 6.x?