NagVis / nagvis

Visualization addon for your open source monitoring core
http://nagvis.org/
GNU General Public License v2.0
113 stars 73 forks source link

PHP8.2 / "Creation of dynamic property ... is debricated" #345

Closed schnudd31do3 closed 11 months ago

schnudd31do3 commented 1 year ago

After upgrading to PHP8.2, Nagvis is not working anymore. This behave is also existent in the latest release 1.9.35.

You get tens of error messages like "Creation of dynamic property ... is debricated". After patching a couple of php files with "#[AllowDynamicProperties]" in front of the faulty class definition, I got it working.

Those files had to be patched: /usr/share# ./nagvis/share/server/core/classes/CoreModManageBackgrounds.php:#[AllowDynamicProperties] ./nagvis/share/server/core/classes/CoreAuthModPDO.php:#[AllowDynamicProperties] ./nagvis/share/server/core/classes/ViewManageBackends.php:#[AllowDynamicProperties] ./nagvis/share/server/core/classes/CoreExceptions.php:#[AllowDynamicProperties] ./nagvis/share/server/core/classes/ViewMapAddModify.php:#[AllowDynamicProperties] ./nagvis/share/server/core/classes/CoreModMap.php:#[AllowDynamicProperties] ./nagvis/share/server/core/classes/objects/NagVisObject.php:#[AllowDynamicProperties] ./nagvis/share/server/core/classes/GlobalMainCfg.php:#[AllowDynamicProperties] ./nagvis/share/server/core/classes/GlobalBackendPDO.php:#[AllowDynamicProperties] ./nagvis/share/server/core/classes/CoreModManageShapes.php:#[AllowDynamicProperties] ./nagvis/share/server/core/classes/CoreModGeneral.php:#[AllowDynamicProperties] ./nagvis/share/server/core/classes/CoreModMainCfg.php:#[AllowDynamicProperties] ./nagvis/share/server/core/classes/GlobalMapCfg.php:#[AllowDynamicProperties] ./nagvis/share/server/core/classes/CorePDOHandler.php:#[AllowDynamicProperties] ./nagvis/share/frontend/nagvis-js/classes/FrontendModOverview.php:#[AllowDynamicProperties] ./nagvis/share/frontend/nagvis-js/classes/FrontendModMap.php:#[AllowDynamicProperties]

tfmotu commented 1 year ago

Hi, only notify that I have the same problem after upgrade to php8.2. @schnudd31do3 if you can share the modified files I will test them in my environment too. Thanks in advance.

schnudd31do3 commented 1 year ago

Hi, as I wrote in my initial post. Insert the "#[AllowDynamicProperties]" exactly before the line "class xxxxx.." in each of the listed files from my post.

schnudd31do3 commented 1 year ago

I know that my modification is only a patch. The newly created version 1.9.36 has the issue, too. I guess the code has to be rewritten in the named files from my first post according to e. g. this: https://medium.com/@parvej.code/deprecations-in-upcoming-php-8-2-and-what-you-have-to-know-1911c93d0483

schnudd31do3 commented 1 year ago

I additionally found that dynamic properties in classes will fail in PHP 9: https://medium.com/@dotcom.software/no-more-dynamic-properties-in-php-9-the-language-is-evolving-260fd70da5e8

So, the code totally has to be rewritten (at the relevant positions) without the usage of dynamic properties in order to be ready also for PHP9.

LarsMichelsen commented 1 year ago

Haven't had a deep look into this, but it seems like it needs some more work. Of course we could temporarily allow all the dynamic properties which are there currently. But as you already wrote, this will not be a long term solution.

I bet some cases can easily be cleaned up by simply declaring the properties. Some others might need a bit more restructuring. I welcome PRs to fix some of these cases.

Maybe I'll find some time to look into that myself, but currently it does not look like I would have much time for that.

schnudd31do3 commented 1 year ago

Thank you for your open and honest answer.

The problem is that Nagvis and PHP version 8.2 shipped with Debian Bookworm do not go together. So I have to stick with the old Bullseye Debian version.

My suggestion for a short term solution is to merge my suggested patches into a new version 1.9.37 so people can migrate their Nagvis installation from Bullseye to Bookworm.

This gave you enough time to create a sustainable solution that can then be incorporated into the next Debian release.

robin-checkmk commented 1 year ago

@schnudd31do3 I cannot find a pull request with your changes. Can you open one, so Lars can easily merge it? That'd be great! :pray:

LarsMichelsen commented 1 year ago

I have now spent some time on this. In the basic classes I added definitions for a few attributes. In other classes I added suppressions.

The problem with this kind of error is that it is reported on access, when the problematic code is executed. So depending on the use case you will trigger the one or the other error. For example: I don't use the PDO backend stuff, so I won't be able to trigger the PDO specific issues. This needs some help from other users.

In current master we have these exceptions:

> grep -R AllowDynamicProperties *
share/server/core/classes/objects/NagVisObject.php:#[AllowDynamicProperties]
share/server/core/classes/CorePDOHandler.php:#[AllowDynamicProperties]
share/server/core/classes/CoreAuthModPDO.php:#[AllowDynamicProperties]
share/server/core/classes/GlobalBackendPDO.php:#[AllowDynamicProperties]

NagVisObject needs to stay like this for the moment, since there is a large number of dynamic attributes in the sub classes of this base class. This needs some detailed analysis and bigger rework to be fixed.

However, the other classes might be easy to improve. One only needs to remove the AllowDynamicProperties annotation from the code to trigger the specific error cases. Then one could add the attributes that are needed to the class.

With the current master state I was able to create a basic map and use it over here with the livestatus backend.

Feel free to give it a try and if you find remaining issues (which is very likely), please send a pull request.

Generally speaking there are static analyzers to find issues like this. However, NagVis has not been built with tool support nor made ready for it. So the tools can not easily be used with NagVis.

schnudd31do3 commented 1 year ago

Hi thanks. I tried your last master state and I found it not working for e. g. /nagvis/frontend/nagvis-js/index.php?mod=Map&act=view&show=Virtual_Server_Machines

I have prooven it, that I need to have modified ALL of my initial mentioned files. I have also prooven it for PHP <= V8.1. The patch is tolerant and does not make any errors.

I sent you a download link for a zipped navgis with my modifications, see the first post. Additionally I created a pullrequest: dynamic-class-property, but I am not sure if this is the right way. I am not familiar with it.

LarsMichelsen commented 12 months ago

I have now applied the more specific fixes to the master branch without using the suppressions (AllowDynamicProperties) that were suggested before.

As I said before I can not be 100% sure that I found all cases. So the next step is now to try the changes I made and report every specific remaining error. I'll need the exact error message to fix the remaining issues.

schnudd31do3 commented 12 months ago

I exchanged file by file from the master branch to my 1.9.35 installation. On exchange of the GlobalMainCfg.php, I got this error: image

Without this new file, I got this: image

LarsMichelsen commented 12 months ago

This looks like an unrelated SQLite issue that your DB can not be written.

schnudd31do3 commented 12 months ago

I solved it. It was permission problems.

Some selectable item seems to be to wide. image image

schnudd31do3 commented 11 months ago

Hi there, no comments for my second last post?

LarsMichelsen commented 11 months ago

Good that the PDO issue was resolved. I'll then to a NagVis release now to get the PHP 8.2 fixes out of the door. Then let's see how it goes.

Besides that, please don't mix topics. If you find unrelated issues, open a new issue. This helps keeping track of things.

Second, don't push your topics. It won't help. Most people in open source are contributing in their spare time and most have some other things to do. So if you need things to be cared for faster, better find someone to pay or try to find a solution and send a pull request.