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

ENH Fix deprecation issues for PHP 8.1 compatibility #10273

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Apr 5, 2022

Issue #10250

Manually fixing various issues related to php8.1

auto_detect_line_endings deprecated

The ini setting auto_detect_line_endings is deprecated as of php 8.1

This is set in a couple of places for csv handling and is blocks php 8.1 compatibility

The option is no longer required

https://php.watch/versions/8.1/auto_detect_line_endings-ini-deprecated

strftime deprecated

Replaced with date()

https://php.watch/versions/8.1/strftime-gmstrftime-deprecated

Passing null to parameter deprecated

A few instances of null being used instead of '' for a blank parameter

https://php.watch/versions/8.1/internal-func-non-nullable-null-deprecation

Note: the bulk of this conversion work was done automatically, links to PRs are on the parent issue

Casting ctype_digit param to string

The param type is mixed, though from php 8.1 passing a non-string param is deprecated

https://www.php.net/manual/en/function.ctype-digit.php

Calling static methods on traits

https://www.php.net/manual/en/migration81.deprecated.php#migration81.deprecated.core.static-trait

@emteknetnz emteknetnz marked this pull request as draft April 5, 2022 02:09
@emteknetnz emteknetnz force-pushed the pulls/4/remove-ini-setting branch from b3fda33 to 2afac0a Compare April 5, 2022 02:48
@emteknetnz emteknetnz changed the title ENH Do not set deprecated auto_detect_line_endings ENH Fix deprecation issues for php 8.1 compatibility Apr 5, 2022
@emteknetnz emteknetnz changed the title ENH Fix deprecation issues for php 8.1 compatibility ENH Fix deprecation issues for PHP 8.1 compatibility Apr 5, 2022
@emteknetnz emteknetnz force-pushed the pulls/4/remove-ini-setting branch 4 times, most recently from 71b0220 to 7c8b9a6 Compare April 5, 2022 09:25
@emteknetnz emteknetnz marked this pull request as ready for review April 5, 2022 21:18
@emteknetnz emteknetnz force-pushed the pulls/4/remove-ini-setting branch from 7c8b9a6 to 814c5b2 Compare April 5, 2022 23:34
Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

I prefer the $class::has_extension(MyExtension::class) syntax to the ViewableData::has_extension($class, MyExtension::class).

Mostly, I don't like that in theory that ViewableData could be substituted with any other class that uses the Extensible trait. And the one parameter syntax is a bit nicer. Admittedly, that's a personal taste.

I'm not dead set against ViewableData::has_extension($class, MyExtension::class) and I'm happy to use that if people think it make more sense.

Mostly, I just want us to pick one option and stick with it everywhere.

@@ -56,7 +56,7 @@ class GeocodableDataObject implements ModelTypePlugin
);

// only apply the plugin to geocodable DataObjects
if (!Extensible::has_extension($class, Geocodable::class)) {
if (!ViewableData::has_extension($class, Geocodable::class)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest using this syntax instead because ViewableData does not inherently have anything to do with the current context. But in the grander scheme, it doesn't really matter.

Suggested change
if (!ViewableData::has_extension($class, Geocodable::class)) {
if (!$class::has_extension(Geocodable::class)) {

Copy link
Member Author

@emteknetnz emteknetnz Apr 6, 2022

Choose a reason for hiding this comment

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

We don't know ahead of time if $class has the Extensible trait, so we'd need something like this

method_exists($class, 'has_extension') && call_user_func("$class::has_extension", $class, Geocodable::class)

Which is very verbose. Therefore I think it's better to simply use

ViewableData::has_extension($class, Geocodable::class)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair point.

@@ -85,7 +85,7 @@ class GeocodableQuery implements ModelQueryPlugin
{
$class = $query->getModel()->getSourceClass();
// Only apply to geocodable objects
if (!Extensible::has_extension($class, Geocodable::class)) {
if (!ViewableData::has_extension($class, Geocodable::class)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!ViewableData::has_extension($class, Geocodable::class)) {
if (!$class::has_extension(Geocodable::class)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

As above

@@ -177,7 +177,7 @@ public function apply(ModelQuery $query, Schema $schema, array $config = []): vo
{
$class = $query->getModel()->getSourceClass();
// Only apply to geocodable objects
if (!Extensible::has_extension($class, Geocodable::class)) {
if (!ViewableData::has_extension($class, Geocodable::class)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!ViewableData::has_extension($class, Geocodable::class)) {
if (!$class::has_extension(Geocodable::class)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

As above

@@ -597,7 +598,7 @@ public static function classesWithExtension(

// only keep classes with the Extension applied
$classes = array_filter($classes, function ($class) use ($extensionClass) {
return Extensible::has_extension($class, $extensionClass);
return ViewableData::has_extension($class, $extensionClass);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return ViewableData::has_extension($class, $extensionClass);
return $class::has_extension($extensionClass);

Copy link
Member Author

Choose a reason for hiding this comment

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

As above

@@ -17,6 +16,7 @@
use SilverStripe\Core\Config\Config;
use SilverStripe\Core\Convert;
use Exception;
use SilverStripe\View\ViewableData;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use SilverStripe\View\ViewableData;

Copy link
Member Author

Choose a reason for hiding this comment

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

As above

@@ -418,7 +418,7 @@ private function getHierarchyBaseClass(): string
{
$ancestry = ClassInfo::ancestry($this->owner);
$ancestorClass = array_shift($ancestry);
while ($ancestorClass && !Extensible::has_extension($ancestorClass, self::class)) {
while ($ancestorClass && !ViewableData::has_extension($ancestorClass, self::class)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
while ($ancestorClass && !ViewableData::has_extension($ancestorClass, self::class)) {
while ($ancestorClass && !$ancestorClass::has_extension(self::class)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

As above

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Objection has been withdrawn.

@maxime-rainville maxime-rainville merged commit 3e5a74c into silverstripe:4 Apr 7, 2022
@maxime-rainville maxime-rainville deleted the pulls/4/remove-ini-setting branch April 7, 2022 01:25
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.

2 participants