[BUG]: Phalcon 5.1.3 and PHP 8.2 dynamic property deprecation warning

[ghost]

Describe the bug
With php 8.2 you can not create dynamic properties, so a controller extending the phalcon controller will throw depcrecation warnings for the injected properties from the interface injectable.

To Reproduce
create a controller and extend from phalcon controller. use asset/view etc.

Expected behavior
no warning should be thrown

Details

• Phalcon version: 5.1.3
• PHP Version: 8.2
• Operating System: ubuntu
• Installation type: package manager
• Zephir version (if any):
• Server: Nginx
• Other related info (Database, table schema): mariadb

[ghost]

This is probably similar to #15973

[rayanlevert]

Just installed Phalcon 5.2.0 with php8.2 and got deprecations to all classes extending Phalcon\Di\Injectable, the attribute #[\AllowDynamicProperties] should be good or implementing the magic method __set() :)

[s-ohnishi]

It seems that it can also be avoided by setting error_reporting(E_ALL ^ E_DEPRECATED);.
This can be done without waiting for a Phalcon update, but it also ignores all other deprecated features.

[stdnk]

Same for me.
PHP 8.2.7
Phalcon 5.2.2

In my opinion ignoring/mute deprecations is bad practice. Hope for a fix soon.

[s-ohnishi]

I agree.
It should not be used except for emergency evacuation.
I also think it should be treated as a high priority bug.

I wrote the following but got no reply.
Discussions/v5.2.2
Discussions/v5.2.1

It is resolved here.
issues/[BUG]: Use of "static" in callables is deprecated
However, I don't know how it was fixed and what version it is.

[Jeckerson]

@s-ohnishi could you provide minimal code example?

[s-ohnishi]

@Jeckerson of course.

mvc/simple-volt is used to indicate
However, it will not work with v5 as it is, so please make the following changes.

Index: simple-volt/public/index.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/simple-volt/public/index.php b/simple-volt/public/index.php
--- a/simple-volt/public/index.php	(revision 277e2f6d53b70d219eb97ca6df60e9280aea08f4)
+++ b/simple-volt/public/index.php	(date 1688995071896)
@@ -25,11 +25,11 @@
     $application = new \Phalcon\Mvc\Application();
     $application->setDI($di);
 
-    $response = $application->handle();
+    $response = $application->handle($_SERVER["REQUEST_URI"]);
 
     $response->send();
-} catch (Phalcon\Exception $e) {
-    echo $e->getMessage();
 } catch (PDOException $e) {
     echo $e->getMessage();
+} catch (Exception $e) {
+    echo $e->getMessage();
 }


Index: simple-volt/app/config/services.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/simple-volt/app/config/services.php b/simple-volt/app/config/services.php
--- a/simple-volt/app/config/services.php	
+++ b/simple-volt/app/config/services.php	(date 1688996529837)
@@ -6,7 +6,8 @@
 use Phalcon\Db\Adapter\Pdo\Mysql as DbAdapter;
 use Phalcon\Mvc\View\Engine\Volt as VoltEngine;
 use Phalcon\Mvc\Model\Metadata\Memory as MetaDataAdapter;
-use Phalcon\Session\Adapter\Files as SessionAdapter;
+use Phalcon\Session\Manager as Session;
+use Phalcon\Session\Adapter\Stream as SessionAdapter;
 use Phalcon\Mvc\View\Engine\Php as PhpViewEngine;
 
 /**
@@ -46,13 +47,13 @@
 
         $view->registerEngines(
             [
-                ".volt" => function ($view, $di) use ($config) {
-                    $volt = new VoltEngine($view, $di);
+                ".volt" => function ($view) use ($config) {
+                    $volt = new VoltEngine($view, $this);
 
                     $volt->setOptions(
                         [
-                            "compiledPath"      => $config->application->cacheDir,
-                            "compiledSeparator" => "_",
+                            "path"      => $config->application->cacheDir,
+                            "separator" => "_",
                         ]
                     );
 
@@ -103,7 +104,11 @@
 $di->set(
     "session",
     function () {
-        $session = new SessionAdapter();
+        $session = new Session();
+        $files = new SessionAdapter([
+            'savePath' => sys_get_temp_dir(),
+        ]);
+        $session->setAdapter($files);
 
         $session->start();
 


Index: simple-volt/app/config/loader.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/simple-volt/app/config/loader.php b/simple-volt/app/config/loader.php
--- a/simple-volt/app/config/loader.php	
+++ b/simple-volt/app/config/loader.php	(date 1688995010840)
@@ -1,13 +1,13 @@
 <?php
 
-use Phalcon\Loader;
+use Phalcon\Autoload\Loader;
 
 $loader = new Loader();
 
 /**
  * We're a registering a set of directories taken from the configuration file
  */
-$loader->registerDirs(
+$loader->setDirectories(
     [
         $config->application->controllersDir,
         $config->application->modelsDir,


Index: simple-volt/app/config/config.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/simple-volt/app/config/config.php b/simple-volt/app/config/config.php
--- a/simple-volt/app/config/config.php	
+++ b/simple-volt/app/config/config.php	(date 1688994957600)
@@ -1,6 +1,6 @@
 <?php
 
-use Phalcon\Config;
+use Phalcon\Config\Config;
 
 return new Config(
     [

Then add the following to views/index.volt:

         <p>{{ tag.a('/', 'link to top', ['class':'btn btn-primary btn-large btn-success'], true) }}</p>
         <p><?= \Phalcon\Tag::linkTo(['/', 'link to top', 'class' => 'btn btn-primary btn-large btn-success']) ?></p>

The old-style tag helpers are on the verge of disappearing, but are still in use at this point.

In this state, when you access simple-volt, it will be displayed as follows.

Deprecated: Creation of dynamic property Phalcon\Mvc\View\Engine\Volt::$tag is deprecated in /var/www/mvc/simple-volt/app/cache/_var_www_mvc_simple-volt_app_views_index.volt.php on line 8
Deprecated: Use of "self" in callables is deprecated in /var/www/mvc/simple-volt/app/cache/_var_www_mvc_simple-volt_app_views_index.volt.php on line 9

[niden]

Controllers are also affected

@fichtner
I tried the patch but to no avail for the following... I'm seeing:

PHP Deprecated: Creation of dynamic property OPNsense\Core\Api\MenuController::$request is deprecated in /usr/local/opnsense/mvc/app/controllers/OPNsense/Base/ApiControllerBase.php on line 195
PHP Deprecated: Creation of dynamic property OPNsense\Core\Api\MenuController::$session is deprecated in /usr/local/opnsense/mvc/app/controllers/OPNsense/Base/ControllerRoot.php on line 149
PHP Deprecated: Creation of dynamic property OPNsense\Core\Api\MenuController::$security is deprecated in /usr/local/opnsense/mvc/app/controllers/OPNsense/Base/ApiControllerBase.php on line 298
It looks to be from the Phalcon\Mvc\Controller class. Any open ticket that tracks these?

Cheers,
Franco

[niden]

Related: zephir-lang/zephir#2405

[niden]

@fichtner Did you try the latest code that is in the v5 branch?

From what I see Controller extends Injectable and that extends stdClass which should solve this issue.

Can you let us know please?

[s-ohnishi]

From what I see Controller extends Injectable and that extends stdClass which should solve this issue.

When will it be released?
After the callables issue was resolved, right?
I have high hopes.

[niden]

We will release this, this weekend maybe earlier to get people going without the DI deprecation warnings. Jeckerson will work on the self:: callable warning through Zephir.

[fichtner]

@niden tried single 0c12a33 on top of 5.2.2 which didn't do the trick. And 5.0.x branch at 1b0390d is still the same: $session, $security and $response are dynamic properties that emit the deprecation warnings.

[dugwood]

@niden

From what I see Controller extends Injectable and that extends stdClass which should solve this issue.

Isn't it a bad practice? As all controllers that extends Controller would hide this very error, making it difficult to debug? That was the purpose of this major PHP change.

So far I don't see a clean fix for this... Perhaps introduce a level with a new property, such as di, which would represent the Di (if Di is extended from stdClass, I think it's not that bad). But that would mean change all $this->view->setVar() to $this->di->view->setVar() and the like.

I've got another issue, still related to all of this:

Deprecated: Creation of dynamic property Phalcon\Mvc\Model\Row::$myproperty is deprecated

(also with ResultSimple)
Extending those from stdClass makes sense... as we can't define the result class, can we?

[s-ohnishi]

I see.
The way Injectable extends stdClass defeats the purpose of updating PHP.
Then, can't it be solved by implementing __set() in Injectable?
If the service is not registered in di, it should be an error (same specification as __get()).
However, if you implement __set() and __get() in a class that extends Injectable, such as in a controller, you must remember to call parent::__set() and parent::__get().

[dugwood]

@s-ohnishi in fact using __set and __get will do approximately the same as extending from stdClass: no more warning. The main issue is that as we always extend from those classes, the warnings disappear too for not Phalcon classes. I really think it's a bad design...
Dependency Injection is great, but as it's too deep in the Phalcon code (all Injectable classes, so at least in all Controllers), it's quite difficult to deal with it without moving to a new Phalcon 6.

@niden @Jeckerson I think it's a major issue, perhaps we should look at other frameworks to see how they handle this (DI + Controller), such as Symfony or Laravel?

Override example (to deal with current issue on controllers):

<?php

class MyController extends Phalcon\Mvc\Controller {

	private $_di;

	public function onConstruct() {
		$this->_di = $this->getDi();
	}

	public function __get($name) {
		return $this->_di[$name];
	}

	public function viewAction() {
		$this->view->setVar('test', 'okay'); // no error on view
	}


}

[s-ohnishi]

@dugwood

in fact using __set and __get will do approximately the same as extending from stdClass

is that so?
If a service is registered or can be registered in Di, it will process it, otherwise "Phalcon" throws an error, so I think I can detect the problem?

[dugwood]

@s-ohnishi you're right, that throws an Exception (Service 'does_not_exists' was not found in the dependency injection container).

I've always had a hard time using this, as if I create a local property that has the same name as a Di service, that goes wrong. But that's by Phalcon's design, so you're right, that should do the trick.

That leaves us with:

• using __get for Controllers/Injectable, as I don't see the use of __set (action would always be on the service or local property, so there's no setting needed)
• adding stdClass for ResultSimple and the like
• fixing self in Volt (such as Phalcon\Tag calls)

[s-ohnishi]

@dugwood
The reason why __set() is not used is probably that basically DI should have been set before the application is called.
If it's not inside "Phalcon", we might be able to handle properties very carefully.

If the self problem only occurs with Phalcon\Tag, then it might be a good idea to migrate to TagFactory entirely...
The impact is too great to be discarded.
It was said that it was "remained for compatibility", but it seems that Forms is also using it...

[niden]

From what I see Controller extends Injectable and that extends stdClass which should solve this issue.

Isn't it a bad practice? As all controllers that extends Controller would hide this very error, making it difficult to debug? That was the purpose of this major PHP change.

In general, dynamic properties are a bad practice. One should always define what they want, its type and keep its scope limited to what it should be doing and accessing.

However, we do not live in an ideal world and shortcuts do appear for convenience. IMHO the best way for this to be developed would have been through an internal array that would keep all the services and their names and utilize __set and __get to access them as magic properties to the children class. A method would have done the same and would have been a bit better say $controller->getService("session"), but $controller->session is more convenient for developers.

Because we did not use the above and used dynamic properties, we are stuck with this. A few other frameworks are doing the same so they have the same issue.

Extending stdClass allows for keeping the current functionality and without changing anything in existing applications. Either way we were creating properties on the fly to allow access to services, so bad practice or not, we cannot change that because pretty much all applications will break.

Although we get rid of the issue with DI related properties not throwing a warning, we indeed are hiding potential errors that might come with a simple typo say ->sesion vs ->session. That, sadly, has always been the case with the implementation even before PHP 8.2

For the future we will make changes with the way the framework is structured but that will take a few versions to materialize. I will need to do a lot of testing to "remove" the stdClass and say use an array that will have stricter controls there for typos or missing services. I did try it once but most things broke in the framework so I left it :/

As for Tag, Forms already uses the TagFactory. A couple things we are missing there: the Select with a db bind that Tag:: has and some additional functionality for radios and checkboxes.

[s-ohnishi]

@niden

Extending stdClass allows for keeping the current functionality and without changing anything in existing applications.

If __set() is implemented in Injectable and only registered services are allowed, there is no need to extend stdClass, so it is not allowed unlimitedly and it is easy to find mistakes, and rewriting of the program can be minimized, right?

As for Tag, Forms already uses the TagFactory.

I forgot, only a few of them used Phalcon\Tag.

[niden]

That is what I tried with the internal array (as mentioned in the reply above) and it failed miserably. I was getting errors from the CLI that the MainTask class could not be loaded. That was a huge rabbit hole I did not have the time to dive into.

I will definitely work something out later on once we "calm" down from all the issues we have.

[s-ohnishi]

understood.
I will wait with anticipation.

[kunalray1993]

The same error is also in the latest version. I'm using PHP v8.2.13 & Phalcon v5.4.0.

[s-ohnishi]

I heard that it was fixed, so I tried it with v5.5.0, but it still hasn't been improved.

[stumpdk]

This is also a problem in Models with dynamic properties/where properties has not been defined.
Is it recommend to have model properties defined in the class, or should the framework be able to handle this being set dynamically?

[svdigital-development]

Hello, any update on this issue?

It's impossible to me to upgrade to php 8.2 just because of this error... Running PHP 8.2.12 with Phalcon 5.6.0.

Thank you

[s-ohnishi]

I understand that there are various circumstances, but I think it's about time for some progress to be made on this issue.
The time has already come to support PHP8.3 ( not PHP8.2).

[niden]

This has been fixed for PHP 8.2+ and above

zephir-lang/zephir#2405