Skip to content
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

Use the $ThemeDir variable instead of a static path #67

Merged
merged 2 commits into from
Apr 28, 2019
Merged

Use the $ThemeDir variable instead of a static path #67

merged 2 commits into from
Apr 28, 2019

Conversation

manuth
Copy link
Contributor

@manuth manuth commented Apr 15, 2019

In this PR I replaced the static path to the favicon with the currently deprecated $ThemeDir variable.
Another possibility would be to use $resourceUrl("silverstripe-themes/simple:{ path }") instead.

This PR fixes issue #66.

@manuth
Copy link
Contributor Author

manuth commented Apr 15, 2019

I've just noticed that $ThemeDir has been in use before already, which has been patched in PR #48.
I think using a static path instead would cause trouble whenever the directory-structure of silverstripe may change. Using $ThemeDir would ensure the path is correct in every possible case.

@robbieaverill
Copy link
Contributor

Yeah using $resourceURL() is the correct way to fix this. This is so that when the directory structure does change (via enabling or disabling the public webroot) the resource reference can still be resolved dynamically. Would you mind updating the PR to use it?

@manuth
Copy link
Contributor Author

manuth commented Apr 17, 2019

I have noticed that $resourceURL() does not seem to work as silverstripe-themes/simple is not included in ModuleLoader::inst()->getManifest()->getModules().

According to the comments only projects with an _config.php-file in it are considered as an actual module.

Does that mean, themes are generally not considered as modules?
Because I haven't seen a theme containing a _config.php-file yet.

@manuth
Copy link
Contributor Author

manuth commented Apr 17, 2019

As this is an issue related to the silverstripe-framework I now created an issue in the proper repository:
silverstripe/silverstripe-framework#8936
silverstripe/silverstripe-framework#7592

The issue can be solved as soon as said issue of the silverstripe-framework is fixed.

Until that, using $ThemeDir seems to be the only way to go.

@robbieaverill
Copy link
Contributor

For what it's worth, we use $resourceURL in our CWP theme templates by referencing a kind of relative path: https://github.com/silverstripe/cwp-starter-theme/blob/master/templates/Includes/Favicon.ss

$resourceURL('cwp/starter-theme: ico/favicon.ico') would be a cleaner solution, but the above works.

@manuth
Copy link
Contributor Author

manuth commented Apr 23, 2019

Humh... Fair, this is another good approach.
But this does not cover the case that, for instance, the name of the theme or the path to the themes-directory is changed in a future release, which would be covered by using $ThemeDir.

In issue silverstripe/silverstripe-framework#7592 they are looking for some good replacement for $ThemeDir.

@tractorcow
Copy link
Contributor

ThemeDir is deprecated, so we shouldn't be using it.

ModuleLoader doesn't work for themes though, unfortunately, @robbieaverill . Unless something's changed. :P

@tractorcow
Copy link
Contributor

Use <link rel="shortcut icon" href="$resourceURL('themes/simple/images/favicon.ico')" /> instead.

Copy link
Contributor

@tractorcow tractorcow left a comment

Choose a reason for hiding this comment

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

As commented, please don't use deprecated $ThemeDir.

You can use <link rel="shortcut icon" href="$resourceURL('themes/simple/images/favicon.ico')" /> instead.

@tractorcow tractorcow merged commit dd28f3f into silverstripe:master Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants