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" #28

Merged
merged 5 commits into from
Jun 21, 2022

Conversation

JoelAlphonso
Copy link
Contributor

@JoelAlphonso JoelAlphonso commented Jun 10, 2022

Based on locomotivemtl/charcoal-app#10


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:

  • 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.
  • charcoal/core
    • Support removed trailing slash in base path for metadata directories in MetadataLoader.
  • charcoal/property
    • Replace "public access" concept with filesystem paths in FileProperty and ImageProperty.
  • charcoal/translator
    • Update relative cache path in TranslatorConfig.
    • Support removed trailing slash in base path for translation directories in TranslatorServiceProvider.
  • charcoal/view
    • Update relative cache paths in view engines.
    • Support removed trailing slash in base path for view directories in ViewServiceProvider and AbstractLoader.

JoelAlphonso and others added 5 commits June 10, 2022 14:47
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.
@JoelAlphonso JoelAlphonso force-pushed the mcaskill-patch-from-base-path branch from ba7d08e to 4508be0 Compare June 10, 2022 19:03
@mcaskill mcaskill changed the title Mcaskill patch from base path Change usage of "base_path" and "public_path" Jun 10, 2022
@mcaskill mcaskill requested a review from mducharme June 10, 2022 20:46
@JoelAlphonso JoelAlphonso force-pushed the mcaskill-patch-from-base-path branch from 70f483d to 4508be0 Compare June 13, 2022 14:41
@JoelAlphonso
Copy link
Contributor Author

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@JoelAlphonso JoelAlphonso mentioned this pull request Sep 15, 2022
charcoal-butler bot pushed a commit that referenced this pull request Sep 15, 2022
## [3.1.8](v3.1.7...v3.1.8) (2022-09-15)

### ⚠ BREAKING CHANGES

* **elfinder:** removed default base_path and public_path. These config keys should be defined in the AppConfig initialization. `new AppConfig(['base_path' => '...']);`

### Bug Fixes

* **base-path:** fix base_path concatenation issues since changes made to AppConfig see #28 (comment) ([84441a3](84441a3)), closes [/github.com//pull/28#issue-1267893850](https://github.com/charcoalphp//github.com/charcoalphp/charcoal/pull/28/issues/issue-1267893850)
* **bin:** fix charcoal binary appConfig's basePath ([95bd3a5](95bd3a5))
* **elfinder:** fix issues with elfinder paths ([a476327](a476327))
charcoal-butler bot pushed a commit that referenced this pull request Sep 21, 2022
## [4.0.0](v3.1.7...v4.0.0) (2022-09-21)

### ⚠ BREAKING CHANGES

* **elfinder:** removed default base_path and public_path. These config keys should be defined in the AppConfig initialization. `new AppConfig(['base_path' => '...']);`

### Bug Fixes

* **base-path:** fix base_path concatenation issues since changes made to AppConfig see #28 (comment) ([84441a3](84441a3)), closes [/github.com//pull/28#issue-1267893850](https://github.com/charcoalphp//github.com/charcoalphp/charcoal/pull/28/issues/issue-1267893850)
* **bin:** fix charcoal binary appConfig's basePath ([95bd3a5](95bd3a5))
* **elfinder:** fix issues with elfinder paths ([a476327](a476327))
* **releaserc:** add parserOpts for commit analyser ([b004e04](b004e04))
@mcaskill mcaskill deleted the mcaskill-patch-from-base-path branch November 12, 2022 19:45
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