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

Adding dynamic properties by Registrars will be deprecated on PHP 8.2 #6162

Closed
kenjis opened this issue Jun 20, 2022 · 10 comments
Closed

Adding dynamic properties by Registrars will be deprecated on PHP 8.2 #6162

kenjis opened this issue Jun 20, 2022 · 10 comments
Labels

Comments

@kenjis
Copy link
Member

kenjis commented Jun 20, 2022

Registrars can add dynamic properties now.

But the sample code in the user guide will cause the error on PHP 8.2.
https://codeigniter4.github.io/CodeIgniter4/general/configuration.html#explicit-registrars

bash-3.2$ php public/index.php 


[ErrorException]

Creation of dynamic property Config\MySalesConfig::$actual is deprecated

at SYSTEMPATH/Config/BaseConfig.php:211

Backtrace:
  1    SYSTEMPATH/Config/BaseConfig.php:211
       CodeIgniter\Debug\Exceptions()->errorHandler(8192, 'Creation of dynamic property Config\\MySalesConfig::$actual is deprecated', '.../CodeIgniter4/system/Config/BaseConfig.php', 211)

  2    SYSTEMPATH/Config/BaseConfig.php:64
       CodeIgniter\Config\BaseConfig()->registerProperties()

  3    APPPATH/Controllers/Home.php:9
       CodeIgniter\Config\BaseConfig()->__construct()

  4    SYSTEMPATH/CodeIgniter.php:896
       App\Controllers\Home()->index()

  5    SYSTEMPATH/CodeIgniter.php:466
       CodeIgniter\CodeIgniter()->runController(Object(App\Controllers\Home))

  6    SYSTEMPATH/CodeIgniter.php:349
       CodeIgniter\CodeIgniter()->handleRequest(null, Object(Config\Cache), false)

  7    FCPATH/index.php:55
       CodeIgniter\CodeIgniter()->run()
bash-3.2$ php -v
PHP 8.2.0-dev (cli) (built: Jun 20 2022 00:32:41) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.2.0-dev, Copyright (c) Zend Technologies
    with Zend OPcache v8.2.0-dev, Copyright (c), by Zend Technologies

References:

@iRedds
Copy link
Collaborator

iRedds commented Jun 20, 2022

#[AllowDynamicProperties]

@kenjis
Copy link
Member Author

kenjis commented Jun 20, 2022

The issue is we will continue to use Dynamic Properties that PHP will denies by default or not.

@iRedds
Copy link
Collaborator

iRedds commented Jun 20, 2022

Why not use the Entity::$attributes case?

@kenjis kenjis mentioned this issue Jun 21, 2022
7 tasks
@kenjis
Copy link
Member Author

kenjis commented Jun 22, 2022

Why not use the Entity::$attributes case?

What do you mean?
Do you mean that we should change it to use magic properties?

@kenjis
Copy link
Member Author

kenjis commented Jun 22, 2022

I don't know the needs to add properties dynamically to Config objects.
I think dynamic properties are bad practice. So it will be rejected by default in PHP in the future.

We can override config properties by Registrars now. What's missing?

@iRedds
Copy link
Collaborator

iRedds commented Jun 23, 2022

What do you mean?
Do you mean that we should change it to use magic properties?

That is, all properties not previously defined should be placed in a separate container, which will have access through magic methods.

I'm just suggesting solutions to the problem in 8.2. This does not mean that I agree with the use of dynamic properties.

@kenjis
Copy link
Member Author

kenjis commented Jun 23, 2022

@iRedds Okay, thanks for explanation. It is an option.

It seems there are three options.

  1. Use #[AllowDynamicProperties]
  2. Use magic properties
  3. Do not support dynamic property injection

I think 3. is simple and preferable.

I did not know that Registrars can add dynamic properties.
Not documented until recently. See #6160

@kenjis
Copy link
Member Author

kenjis commented Jun 27, 2022

See #6172 (comment)

@iRedds
Copy link
Collaborator

iRedds commented Jun 27, 2022

I do not know what to say.
The 8.2 conflicts you mentioned are mostly due to class design.

public function __construct(array $params)
{
foreach ($params as $key => $value) {
$this->{$key} = $value;
}

But the tests/_support/Config/Registrar.php class does not define a new property in the database configuration class.

@kenjis
Copy link
Member Author

kenjis commented Oct 12, 2022

We do not recommend the use of dynamic properties in Registrar.

@kenjis kenjis closed this as completed Oct 12, 2022
@kenjis kenjis added the PHP 8.2 label Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants