PHP 8+ Support #94
PHP 8+ Support #94
Conversation
Add polyfill for strftime
MouseEatsCat
requested review from
a team,
BeneRoch,
JoelAlphonso,
mcaskill and
mducharme
and removed request for
a team and
JoelAlphonso
mcaskill
left a comment
mcaskill
left a comment
There was a problem hiding this comment.
Impressive work overall. Much appreciated.
The quality of some small partions are questionable. How much of this was handled by an LLM vs an IDE vs human?
If there's an easy way to do it, I would recommend cleaning up the PHP imports in all files. Remove the superfluous // From comments and group all imports together and sort them alphabetically.
Service providers should use the type of Charcoal's AppContainer or DI\Container instead of PSR-11's interface since these classes provide the services.
| $container->set('environment', function () use ($path) { | ||
| return [ | ||
| 'PATH_INFO' => $path, | ||
| 'REQUEST_URI' => $path, | ||
| ]; | ||
| }); |
There was a problem hiding this comment.
Could this just assign the array itself?
| $container->set('environment', function () use ($path) { | |
| return [ | |
| 'PATH_INFO' => $path, | |
| 'REQUEST_URI' => $path, | |
| ]; | |
| }); | |
| $container->set('environment', [ | |
| 'PATH_INFO' => $path, | |
| 'REQUEST_URI' => $path, | |
| ]); |
| class AppContainer implements ContainerInterface | ||
| { | ||
| /** | ||
| * @var Container | ||
| */ | ||
| private Container $container; |
There was a problem hiding this comment.
It might be better to extend PHP-DI's Container instead of implementing PSR-11's interface and using PHP-DI internally.
| // This method is a stub. Reimplement in children action classes. | ||
| } | ||
|
|
||
| public static function getParams(ServerRequestInterface $request, ?array $keys = []): array |
There was a problem hiding this comment.
In Slim v4, they decoupled their PSR-7 implementation (slim/psr7) from their decorations (slim/http).
I would recommend using Slim's HTTP decorators to maintain compatibility with our existing misuse of Slim's HTTP implementation.
|
|
||
| if (!empty($this->action)) { | ||
| $return = $this->{$this->action}($this->collection, $text); | ||
| } | ||
|
|
||
| $text = ($return ?? ''); |
There was a problem hiding this comment.
The empty() call is unnecessary because the $action property is explicitly declared on the class.
| if (!empty($this->action)) { | |
| $return = $this->{$this->action}($this->collection, $text); | |
| } | |
| $text = ($return ?? ''); | |
| if ($this->action) { | |
| $text = ($this->{$this->action}($this->collection, $text) ?? ''); | |
| } |
There was a problem hiding this comment.
Add type declarations to Charcoal's translation methods.
This snippet ($domain ?? null) occurs multiple times and seems unnecessary given $domain is expected to be string|null.
| @@ -22,6 +22,7 @@ | |||
| * - Provides the ability for a store to fetch data in another store. | |||
| * - Provides this store with a way to register one or more delegate stores. | |||
| */ | |||
| #[\AllowDynamicProperties] | |||
There was a problem hiding this comment.
Is this attribute necessary if its been applied to its ancestor (AbstractEntity in this scenario)?
| /** | ||
| * Serialize the object (PHP < 7.4 compatibility). | ||
| * | ||
| * @return string | ||
| */ | ||
| public function serialize(): string | ||
| { | ||
| if (method_exists($this, '__serialize')) { | ||
| return serialize($this->__serialize()); | ||
| } | ||
| // Fallback for older PHP | ||
| return serialize([ | ||
| 'condition' => $this->condition, | ||
| ]); | ||
| } | ||
|
|
||
| /** | ||
| * Unserialize the object (PHP < 7.4 compatibility). | ||
| * | ||
| * @param string $data | ||
| * @return void | ||
| */ | ||
| public function unserialize($data): void | ||
| { | ||
| $values = unserialize($data); | ||
| if (method_exists($this, '__unserialize')) { | ||
| $this->__unserialize($values); | ||
| } else { | ||
| $this->condition = ($values['condition'] ?? null); | ||
| } | ||
| } |
There was a problem hiding this comment.
What's the scenario for un/serialization?
| @@ -72,7 +72,8 @@ public function emailFromArray(array $arr): string | |||
| return $email; | |||
| } | |||
|
|
|||
| $name = str_replace('"', '', filter_var($arr['name'], FILTER_SANITIZE_STRING)); | |||
| $name = isset($arr['name']) ? strip_tags(trim($arr['name'])) : null; | |||
There was a problem hiding this comment.
Trim after stripping tags.
There was a problem hiding this comment.
Why is there a need for CSV validating and sanitization?
The file ends being processed 3 times (twice by Charcoal to validate then sanitize it, then once more by Symfony to import localizations).
| /*echo '<pre>'; | ||
| print_r($container->get('view/loader/dependencies')); | ||
| echo '</pre>'; | ||
|
|
||
| foreach ($container->get('view/loader/dependencies')['paths'] as $path) { | ||
| $fullPath = $container->get('view/loader/dependencies')['base_path'] . DIRECTORY_SEPARATOR . $path . 'login.mustache'; | ||
| echo $fullPath . '<br>'; | ||
| if (file_exists($fullPath)) { | ||
| exit('Found template at: ' . $fullPath); | ||
| } | ||
| } | ||
| exit;*/ |
🚧 WIP 🚧
This update makes charcoal compatible with PHP 8+.
Slim was upgraded to version 4 and this caused a lot of breaking changes.
Biggest Changes (Breaking)
Container
Slim 4 no longer comes with its own Dependency injection container. Also, Pimple is no longer compatible with PSR-7's ContainerInterface. Because of this, I've decided to go with PHP-DI as a container.
This means that the container can no longer be accessed as an array. So the methods:
->get(),->set(),->has()must be used instead. Also the->extend()method is no longer available: Instead you must get the previously set item, modify it, and then set it again.