Leading slash in theme paths

[donquixote]

See my comment in #298 (comment).

I defined a pattern in a module using a yml file, and I noticed that the theme hook had a path starting with a leading slash.
E.g. /modules/custom/my_module/templates/patterns/my_pattern/pattern-my-pattern.html.twig.
The problem seems to be the str_replace() in PatternBase and LibraryPattern, which is meant to remove the absolute part from the pattern path.

$definition_base_path = '/var/www/html/web/modules/custom/my_patterns/templates/patterns/my_pattern';
$root = '/var/www/html/web';
$path = str_replace($root, '', $definition_base_path);
print $path === '/modules/custom/my_patterns/templates/patterns/my_pattern' ? 'problem' : 'ok';

I am a bit confused why this was not reported as broken before. Am I doing something wrong?

Going to post a minimal PR and we'll see which test blows up.

[donquixote]

It seems that in many cases the path with the leading slash is overridden with the correct path, e.g. if the pattern is provided by a theme. This might be the reason why this was undetected so far.

[donquixote]

I would add tests, but we first need to fix existing tests..

[vever001]

It seems there was already a PR that attemps to fix it #327
But that one is not OK IMO as $this->root is used in 2 places:

1. LibraryPattern.php#L81
2. PatternBase.php#L82 - called if the pattern definition specifies a "base path".

Your PR will work in both cases where $this->root is used.
My only concern is that adding a trailing slash doesn't seem to follow the convention.
E.g.: if someone extends PatternBase and expects no trailing slash, and does $this->root . DIRECTORY_SEPARATOR ... it may break.
Maybe we should fix it in both places by adding the slash?

[donquixote]

Maybe we should fix it in both places by adding the slash?

Yes we can do that.
My PR was the most lazy solution possible :)

[donquixote]

It seems there was already a PR that attemps to fix it #327

Good catch! I did not find this one before.
Perhaps we should start from #327, also to give credit to @mistermoper.

[omarlopesino]

Hi.

I've updated the PR with the feedback from #328 (comment):

• Added the slash to PatternBase#82
• Use DIRECTORY_SEPARATOR

Knowing that $this->root comes from the app.root service container parameter, it is reasonable to keep the property without the slash, so that it has an expected value for those who extend PatternBase.