Update charcoal to support PHP 8.3 - 8.5#96
Conversation
Introduced a utility script to streamline monorepo package management. Supports listing packages in default, JSON, or pretty formats.
… package handling
…proved localization Replaced the deprecated `strftime` with `IntlDateFormatter` for date formatting. Introduced locale support to the `DateHelper` class for better internationalization.
…s` and removing redundant parsing
…monorepo-builder` configuration
…ma and cleanup deprecated settings
…ed cache and log files
…or for modern PHP syntax (rector utility) Applied strict typing declarations across packages. Updated composer dependencies to require PHP 8.3 and PHPUnit ^12.5. Refactored codebase with type hints, `static` return type, and improved null checks.
Introduced `$tableExistsCache` for caching table existence checks. Replaced `property_exists` logic with an internal cache and added `#[\AllowDynamicProperties]` for PHP 8.2 compatibility.
…, PHPUnit, and other libraries
…^7.2, Slim ^3.13, Stash ^1.1, and PSR/Cache ^2.0
… add type hints, and refactor for modern PHP syntax - Eliminated `MessageSelector` as it is no longer required with `MessageFormatter`. - Added type hints and return types across various components, improving strict typing consistency. - Simplified logic and migrated to modern PHP syntax for better code maintainability.
…ethods, and modernize for strict typing compliance
…ecated docblocks to modern syntax
…ts, and fix syntax issues for immutability compliance
…nce across CollectionContainerTrait and TableWidget
…ePassByReference`
…pdating parentheses usage, and adding return type hints where applicable
…and clean up unused selectors in translator tests
…asses in UI-related tests
…classes in user-related tests
…classes and clean up deprecated mocks in view-related tests
…uite interruptions
…dist configs and removing unused bootstrap handling
…ernize schema versions
…in GitHub Actions
… for package testing
…clean up unused GitHub Actions step
…nit configurations across all packages
…ions with explicit loops for better compatibility
…clean up #[Override] attributes from class properties
… and update associated configuration
…property namespace
mcaskill
left a comment
There was a problem hiding this comment.
This is a preliminary review wherein I've looked at top-level files, Composer manifests, and PHPUnit configuration files.
I'll submit further reviews as I dive into the packages.
There was a problem hiding this comment.
Add a .gitattributes file and exclude .ddev with export-ignore.
| @@ -1,4 +1,6 @@ | |||
| .phpunit.result.cache | |||
There was a problem hiding this comment.
With .phpunit.cache, I think we can remove .phpunit.result.cache.
| @@ -1,4 +1,6 @@ | |||
| .phpunit.result.cache | |||
| phpunit.xml.dist.bak | |||
There was a problem hiding this comment.
I recommend removing this rule. .bak is a generic extension used by various tools. It should be ignored globally rather than ignored at this specific level and file name.
| # Function to list all monorepo packages in pretty format | ||
| packages_pretty() { | ||
| echo "Monorepo packages:" | ||
| find packages -maxdepth 2 -name "composer.json" -type f | while read -r composer_file; do | ||
| package_dir=$(dirname "$composer_file") | ||
| package_name=$(basename "$package_dir") | ||
| echo " - $package_name" | ||
| done | ||
| } | ||
|
|
||
| # Function to list packages as JSON array | ||
| packages_json() { | ||
| packages=$(find packages -maxdepth 2 -name "composer.json" -type f | while read -r composer_file; do | ||
| package_dir=$(dirname "$composer_file") | ||
| basename "$package_dir" | ||
| done | awk 'BEGIN{printf "["} {printf "%s\"%s\"", (NR>1?",":""), $0} END{printf "]\n"}') | ||
| echo "$packages" | ||
| } | ||
|
|
||
| # Function to list package names only (space-separated) - default | ||
| packages_default() { | ||
| find packages -maxdepth 2 -name "composer.json" -type f | while read -r composer_file; do | ||
| package_dir=$(dirname "$composer_file") | ||
| basename "$package_dir" | ||
| done | tr '\n' ' ' | sed 's/ $/\n/' | ||
| } |
There was a problem hiding this comment.
I would recommend having one function execute the find and extract package names.
I think this command I wrote to be more versatile (the list is also sorted for convenience):
find ./packages -type f -name 'composer.json' \
-maxdepth 2 -exec dirname {} + \
| sort -u \
| xargs basenameFrom that, JSON output becomes:
packages_json() {
packages=$(find_packages | awk 'BEGIN{printf "["} {printf "%s\"%s\"", (NR>1?",":""), $0} END{printf "]\n"}')
echo "$packages"
}Also, I don't think you need to store the result in a variable to echo afterwards?
| echo "Usage: $0 packages [--type=TYPE|-t TYPE]" | ||
| echo "Commands:" | ||
| echo " packages List package names (space-separated, default)" | ||
| echo " packages -t json List packages as JSON array" | ||
| echo " packages -t pretty List all monorepo packages in pretty format" |
There was a problem hiding this comment.
--type (-t) feels like an odd choice for this kind of option. I would recommend --format (-f).
Examples using f:
There was a problem hiding this comment.
According to Rector, couldn't one skip AddOverrideAttributeToOverriddenPropertiesRector?
RectorConfig::configure()
->withSkip([
AddOverrideAttributeToOverriddenPropertiesRector::class,
]);| "charcoal/ui": "^5.1", | ||
| "charcoal/user": "^5.1", | ||
| "guzzlehttp/guzzle": "^6.0 || ^7.0", | ||
| "kriswallsmith/assetic": "^1.4", |
There was a problem hiding this comment.
kriswallsmith/assetic will need to be replaced with assetic/framework.
| "require": { | ||
| "php": "^7.4 || ^8.0", | ||
| "php": "^8.3", | ||
| "ext-json": "*", | ||
| "psr/http-message": "^1.0", | ||
| "charcoal/config": "^5.1", | ||
| "erusev/parsedown": "^1.7" |
There was a problem hiding this comment.
erusev/parsedown should be upgraded to v1.8.0 to resolve PHP 8 deprecations.
mcaskill
left a comment
There was a problem hiding this comment.
This review covers UI, User, and View packages.
There was a problem hiding this comment.
Would it be complicated to import class names instead of using fully qualified names?
The only issue I can think of is it would impact (for humans) the manually curated grouping of imports (which are per package).
| */ | ||
| public function __invoke(string $text, LambdaHelper $helper = null): string | ||
| public function __invoke(string $text = '', ?LambdaHelper $helper = null): mixed |
There was a problem hiding this comment.
Replace mixed with static|string.
| */ | ||
| public function __get(string $macro) | ||
| public function __get(string $macro): mixed |
There was a problem hiding this comment.
Replace mixed with static.
| // Mustache v3 calls callable objects with no arguments when traversing dotted | ||
| // names (e.g. `_t.en`). Return $this so the traversal can continue to __get(). |
There was a problem hiding this comment.
Use block comment for PHPDoc parsers to intercept this comment.
| // Mustache v3 calls callable objects with no arguments when traversing dotted | |
| // names (e.g. `_t.en`). Return $this so the traversal can continue to __get(). | |
| /** | |
| * Mustache v3 calls callable objects with no arguments when traversing dotted | |
| * names (e.g. `_t.en`). Return $this so the traversal can continue to __get(). | |
| */ |
| @@ -14,21 +14,23 @@ | |||
| class DebugHelpers extends AbstractExtension implements | |||
| HelpersInterface | |||
| { | |||
| public $debug; | |||
There was a problem hiding this comment.
The property needs a type, either:
public ?bool $debug = null;
public bool $debug = false;
public mixed $debug = false;
If bool, the null coalescing operator should be removed from the isDebug() method.
If mixed, the property should be cast to bool in isDebug().
| @@ -339,13 +339,9 @@ protected function parseFormGroup($groupIdent, $group) | |||
| * @param array|null $data Optional. The form group data to set. | |||
| * @return FormGroupInterface | |||
| */ | |||
| protected function createFormGroup(array $data = null) | |||
| protected function createFormGroup(?array $data = null) | |||
There was a problem hiding this comment.
Add return type:
| protected function createFormGroup(?array $data = null) | |
| protected function createFormGroup(?array $data = null): FormGroupInterface |
There was a problem hiding this comment.
Missing property types which influence other methods throughout the trait.
private int $position = 0;
private array $structure = [];There was a problem hiding this comment.
Some methods are returning float|int|null where they should be ?int.
Example of a refactored method to enforce int.
public function rowNumColumns(?int $position = null): ?int
{
$position ??= $this->position();
$row = $this->rowData($position);
if ($row === null) {
return null;
}
return (int) array_sum($row['columns']);
}| @@ -260,7 +253,7 @@ public function cellSpan($position = null) | |||
| * @param integer $position Optional. Forced position. | |||
| * @return integer | |||
| */ | |||
| public function cellSpanBy12($position = null) | |||
| public function cellSpanBy12($position = null): null|float|int | |||
There was a problem hiding this comment.
This method should round and clamp its result.
|
|
||
| /** | ||
| * Return a new menu. | ||
| * | ||
| * @param array|\ArrayAccess $data Class dependencies. | ||
| */ | ||
| public function __construct($data) | ||
| public function __construct(?array $data) |
There was a problem hiding this comment.
Is there a way for this method to not support null?
Same concern applies to similar classes that such as AbstractMenuItem.
mcaskill
left a comment
There was a problem hiding this comment.
This review covers Translator package.
| @@ -215,11 +189,11 @@ private function getLanguage(RequestInterface $request) | |||
| * @param RequestInterface $request The PSR-7 HTTP request. | |||
| * @return string | |||
| */ | |||
| private function getLanguageFromHost(RequestInterface $request) | |||
| private function getLanguageFromHost(RequestInterface $request): int|string | |||
There was a problem hiding this comment.
Return type should be string.
return $lang; should maybe be return (string) $lang;.
Maybe also add an annotation to the $hostMap property:
/** @var array<string, string> */
private array $hostMap;There was a problem hiding this comment.
Missing parameter type on $domain throughout a few methods.
There was a problem hiding this comment.
The trait's $translator property should be:
private ?Translator $translator = null;I think we can also remove the exception thrown in translator() in favour of delegating error handling to PHP which will expect the return type.
| */ | ||
| private $container; | ||
| private \Pimple\Container|array|null $container = null; |
There was a problem hiding this comment.
This property is only ever ?Container, I see no occurrence of it as an array.
PHP 8.3 / 8.4 Compatibility
Branch:
joel/php8→mainSummary
Updates all 17 Charcoal packages for PHP 8.3 and 8.4 compatibility, addressing breaking changes and deprecations introduced in recent PHP versions.
Breaking Changes Fixed
strftime()removed in PHP 8.4 (charcoal/cms)strftime()calls inDateHelper.phpwithIntlDateFormatterequivalents (6 occurrences)strftime()was deprecated in PHP 8.1 and removed in PHP 8.4Implicit nullable parameters (all packages)
function foo(Type $param = null)→function foo(?Type $param = null)across all packagesDynamic properties (affected model classes)
#[AllowDynamicProperties]attribute to classes relying on dynamic property assignmentDependency Updates
PHP constraint — updated in all 17
composer.jsonfiles:Dev dependencies (all packages):
phpunit/phpunit^9.5^11.0vimeo/psalm^4.23^5.0phpstan/phpstan^1.7^2.0squizlabs/php_codesniffer^3.6^3.10Testing
phpstan,psalm)Notes
charcoal/app) is EOL but functions on PHP 8.4; a Slim 4 migration is tracked separatelycharcoal/cmsDateHelper.phplocale handling preserves existing output format across all supported locales — thestrftime→IntlDateFormatterreplacement was validated againstfr_CAanden_CAlocale strings