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

Change usage of "base_path" and "public_path" #10

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mcaskill
Copy link
Member

@mcaskill mcaskill commented Aug 3, 2021

This pull request does three things:

  1. Removes the trailing slash from "base_path" and "public_path".
  2. Provides tools for configuring better paths.
  3. Provides "cache_path" and a "logs_path".

Since most PHP functions (and __DIR__) and many third-party librairies return and expect paths without a trailing slash, it makes sense to expect Charcoal to follow that same behavior.

Many paths in Charcoal are defined as relative but to what is not well documented (the "public_path") and does not always work as expected (such as from the Charcoal CLI or a custom PHP script).

When using the Charcoal CLI from the project's base path, we end up with secondary cache and logs directories in the directory above the base path. If that path is not accessible from the current user, Charcoal will throw an error from being unable to create the aforementioned directories/files.

Ideally, for security reasons, relative paths should always be from the "base_path", instead of the "public_path".

One solution, I propose, to assist with configuring relative paths is with "template tags":

Examples
- 'path' => './',
+ 'path' => '%app.public_path%'
- 'path' => '../'
+ 'path' => '%app.base_path%'
- 'stream' => '../logs/charcoal.app.log'
+ 'stream' => '%app.logs_path%/charcoal.app.log'
- 'cache_dir' => '../cache/translator'
+ 'cache_dir' => '%app.cache_path%/translator'
- const DEFAULT_CACHE_PATH = '../cache/mustache'
+ const DEFAULT_CACHE_PATH = '%app.cache_path%/charcoal.app.log'

This is accomplished with a new method AppConfig::resolveValue() that replaces any supported %app.*% tokens:

if (is_string($path)) {
    if (isset($container['config']) && ($container['config'] instanceof AppConfig)) {
        $path = $container['config']->resolveValue($path);
    }
}

As for the new configurable paths, "cache_path" and "logs_path", I propose we follow Symfony's default location:

  • $this->basePath().'/var/cache'
  • $this->basePath().'/var/log'

Both the removal of the trailing slash and recommendations for relative paths requires changes to be made in many Charcoal packages such as:

  • locomotivemtl/charcoal-admin
    • Replace "public access" concept with filesystem paths in ElfinderConnectorAction and UploadImageAction.
    • Support removed trailing slash in paths and unify static cache path in StaticWebsite actions.
  • locomotivemtl/charcoal-core
    • Support removed trailing slash in base path for metadata directories in MetadataLoader.
  • locomotivemtl/charcoal-property
    • Replace "public access" concept with filesystem paths in FileProperty and ImageProperty.
  • locomotivemtl/charcoal-translator
    • Update relative cache path in TranslatorConfig.
    • Support removed trailing slash in base path for translation directories in TranslatorServiceProvider.
  • locomotivemtl/charcoal-view
    • Update relative cache paths in view engines.
    • Support removed trailing slash in base path for view directories in ViewServiceProvider and AbstractLoader.

From AppConfig to normalize filesystem paths.
PHP returns paths without a trailing slash.
Added:
- Method `AppConfig::resolveValue()` to replace "%app.*%" with a value on the `AppConfig`.
- Tags to resolve "base_path", "public_path", "cache_path", and "logs_path".
Changed:
- Default connection paths in `FilesystemConfig` to use "%app.base_path%" and "%app.public_path%".
- Default path in `LoggerConfig` to use "%app.logs_path%".
- Routine to resolve any config parameters in the filesystem `LocalAdapter` path.
- Routine to resolve any config parameters in the logger `StreamHandler` path.
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.

None yet

2 participants