Follow-up: PHP 8.2 support for taxonomy vocabularies

[indigoxela]

Description of the bug

EDIT: based on the discussion in this thread, we decided to allow dynamic properties for vocabularies.

In #5927 we added basic PHP 8.2 support for core, including creating vocabularies. To prevent all sorts of form items to get into the newly created TaxonomyVocabulary object, we check if the property exists in function taxonomy_form_vocabulary_submit(). However, that seems to be the wrong spot.

What about TaxonomyVocabulary::__construct() or taxonomy_vocabulary_load() to prevent nagging when loading a vocab?

I discovered that when checking compatibility with 8.2 in the i18n module, which adds items to the vocab form, which get saved to the config json. So every time a new instance of TaxonomyVocabulary gets created, those config items are also "stuffed" into the object. And that causes PHP nagging, of course.

It might eventually be necessary to allow dynamic properties for TaxonomyVocabulary for other contrib, but not for i18n. This is something to be determined. A quick check via GitHub search found no instance of hook_taxonomy_vocabulary_load in any contrib.

Additional information

Add any other information that could help, such as:

• Backdrop CMS version: latest 1.26 dev
• PHP version: 8.2

[indigoxela]

Hm. Or maybe this class is supposed to have dynamic properties?

There's hook_taxonomy_vocabulary_load() – similar to Entities, which currently isn't used anywhere in core.

This doc example indicates, that dynamic properties should be allowed. If that's the case, we might consider extending stdClass instead of a standalone class.

For comparison with Entity: abstract class Entity extends stdClass implements EntityInterface

In Drupal 7 vocabularies are handled like other entities (via entity_load), but in Backdrop they're just instances of the standalone class, which seems OK, as they're rather simple compared to the fully classed Entity in B.

If my suspicion, that TaxonomyVocabulary should actually allow dynamic properties, turns out to be correct, the solution could be: class TaxonomyVocabulary extends stdClass. If not, we might need some cleanup of config items, before we construct an instance. But then we should also consider dropping hook_taxonomy_vocabulary_load().

[herbdool]

I think we should have it also extend stdClass in this case as well, following Entity class's lead.

If we're allowing dynamic properties then checking for property_exists() makes sense to keep it in the submit handler to remove form items. I'm guessing a dynamic property would pass property_exists()?

[argiepiano]

So, if I understand this correctly, we are discussing two problems here: (1) the fact that vocabularies don't allow dynamic properties, and (2), the fact that extraneous properties added to the vocabulary class are being saved to the file.

My thoughts on (1) is that, I agree that TaxonomyVocabulary should extend sdtClass like Entity. It's often very helpful to add temporary properties to entities, basically as sort of storage (thinking for example of entity's original, which is a property added when you update an entity, or is_new, added temporarily when you create an entity).

As for (2), this is a common issues with config files. When vocabularies were entities in D7, they had schemas, and therefore the CRUD save action always used the schema to map the class property to a column, and basically ignored/didn't save all other properties. There isn't an equivalent mechanism for config files (that is, there is no way to define which properties should be saved).

Using property_exists() works fine in the context of taxonomy_form_vocabulary_submit() but it will not work in the context of TaxonomyVocabulary:save(), since, as @herbdool pointed out, added, temporary dynamics properties will pass that.

So, to me, there are two possible paths: (1) Create a way to define which TaxonomyVocabulary properties should be saved by creating the equivalent of a hook_schema() for it (named, for example, hook_config_property_info()). This hook could be used with other config "pseudo entities" as needed. Or (2), pass the responsibility of cleaning up properties that should not be saved to individual contrib modules that add those temporary properties (like the i18n).

[indigoxela]

I'm guessing a dynamic property would pass property_exists()?

@herbdool I don't think so, no. The properties aren't defined in the class, but get attached on-the-fly.

@argiepiano I'm not sure, if we have two problems. But let's figure out. (Nothing "nasty" gets saved to config, no worries.)

As far as I understand it, the config for vocabs (or anything) is just settings in JSON format. So not everything, that goes to a config file is necessarily supposed to be part of the object (instance) created from that config.
Vocabs are rather simple, compared to Entity. They're not fieldable, have no bundles... So IMO we shouldn't try too hard to make them "look" the same as full Entities.

BTW: if we prevent TaxonomyVocabulary config to save anything but valid properties, then i18n_taxonomy will break. And if we introduce a hook to handle additional properties, it still requires a rewrite.

Re property_exists() in taxonomy_form_vocabulary_submit(). I think, that deserves a second and closer look, now that we learned that dynamic properties might be necessary. We might eventually revert that check. If we extend stdClass, the reason why we used it there, is gone.

[argiepiano]

@indigoxela thanks for clarifying. I wasn't aware that property_exists() returns false for dynamic properties. That's good to know. [EDIT: this is not the case]

Re property_exists() in taxonomy_form_vocabulary_submit(). ... We might eventually revert that check. If we extend stdClass, the reason why we used it there, is gone.

Probably. If we get rid of that check, some "noise" will be added to the class. But that should be OK, since removing that check allows modules to alter the form to add "legitimate" new stuff that should be added to the class.

[argiepiano]

@indigoxela, I'm testing property_exists() in PHP 8.2. It returns TRUE for dynamic properties. So, my guess is I misunderstood you. I'll give this thread a rest and will come back later when I have more time re-read and understand the problem.

I tried this:

class TestClass extends stdClass {

    public $declared = null;
    
}

$testObject = new TestClass;

$testObject->dynamic = 'asdf';
dpm(property_exists($testObject, "dynamic")); // Returns TRUE.

[indigoxela]

@argiepiano after you added it, it's there. But not before. And we'd probably need a way to check, if it's valid to add it.
Both, when creating new instances and when loading from config. I'm not sure, if that's even possible. "dynamic" just means "whatever out-of-the-box without defined type" for anything stuffed into the instance.

[avpaderno]

property_exists() works also for dynamically added properties. The following code prints 1 in any PHP version.

class Test {};

$original = new Test();
$obj = new Test();
$obj->original = $original;
echo property_exists($obj, "original"), "\n";

[indigoxela]

property_exists() works also for dynamically added properties

Yes, after you (dynamically) added it, the property is there. But we can't check in advance. 😉

[avpaderno]

That is true for isset() too: It does not return TRUE before a variable is initialized, so it does not check in advance too.

It is not true that property_exists() returns false for dynamic properties, as a comment said. It returns FALSE for any property that is not initialized; properties defined in a class are initialized when an instance of that class is created (and if a property is defined as public $entityManager, for example, it gets initialized to NULL).

[indigoxela]

@kiamlaluno at the risk of derailing this issue a bit 😉, I'll try to explain what we're trying to solve.

<?php
error_reporting(E_ALL);

class Foo {
  public $validprop;
  protected $alsovalid;
}

$instance = new Foo;

var_dump(property_exists($instance, 'validprop'));
var_dump(property_exists($instance, 'alsovalid'));
var_dump(property_exists($instance, 'something'));
// true, true, false

// This is deprecated as of PHP 8.2, it still works, but we don't want nagging. Our tests fail on that.
$instance->something = 'hey';
// PHP Deprecated:  Creation of dynamic property Foo::$something is deprecated
var_dump(property_exists($instance, 'something'));
// true

?>

property_exists() works fine, after the property (dynamic or not) has been set, but we're trying to prevent arbitrary stuff (from config or form submissions) to get added to the object, if possible. And that's tricky when dynamic properties are allowed, for instance when we extend stdClass.

So, if we switch to....

class Foo extends stdClass {

The nagging goes away, but we can't reliably tell, whether a property is valid, or just garbage from config or form submission.

Very likely (based on discussion here so far) we'll allow dynamic properties for vocabs.

class TaxonomyVocabulary extends stdClass {

This is future-proof to prevent nagging or failure with dynamic properties, but we probably have to live with anything getting part of the object via TaxonomyVocabulary::__construct or later on.

Example: taxonomy_vocabulary_load_multiple() reads all vocab configs and builds the objects from anything in there.
Another example: taxonomy_form_vocabulary_submit() currently does a check with property_exists() (this has recently been added), but we have to figure out, if we can keep that, when we explicitly allow dynamic properties.

But everything's fine now, no tests fail on 8.2? Yes, but only, because we currently don't have a functional test for hook_taxonomy_vocabulary_load yet. As soon as we add one (which I'm planning), it will fail on 8.2. 😉

[avpaderno]

@indigoxela I understood the issue purpose; my comment was for another comment.

Using property_exist($object, 'property') is like using isset($object->property), with the exception that the first would return TRUE even when $object->property has been set to NULL. (This can be verified with $object = new stdClass(); $object->property = NULL; echo (int) property_exists($object, "property"), "\n"; echo (int) isset($object->property), "\n";.)
Neither property_exists() nor isset() are used to understand if a property is a dynamic one. For that, it is necessary to use reflection or, in alternative, code similar to the following one, which can verify if TaxonomyVocabulary::$property is a dynamic property.

// Create a new TaxonomyVocabulary object which is not altered by other hooks/functions/methods.
$vocabulary = new TaxonomyVocabulary();
if (property_exists($vocabulary, 'property') {
  // The property is not a dynamic one.
}

[argiepiano]

Thanks for taking the time to expand, @indigoxela. I think we are in agreement that TaxonomyVocabulary should allow dynamic properties. I'd love to see proof of the problem you are trying to solve when this happens, specifically related to hook_taxonomy_vocabulary_load and to the use case with i18n.

@kiamlaluno, no need to keep insisting on correcting my comment. It was wrong. I already edited it (yesterday) by crossing it out and putting an edit note.

[avpaderno]

@argiepiano I was not correcting your comment for the second time; in fact, I only said my comment was for another comment.

I was replying to another comment, posted by somebody else, which seemed to misunderstand my previous comment. In particular, I was replying to property_exists() works fine, after the property (dynamic or not) has been set, but we're trying to prevent arbitrary stuff (from config or form submissions) to get added to the object, if possible. That's all.

[indigoxela]

Here we go, first a failing test (with dynamic property via hook_taxonomy_vocabulary_load()), then the fix (extending stdClass).

[indigoxela]

Now I revisited taxonomy_form_vocabulary_submit() in which we recently added a property_exists() check on form submissions ($form_state['values'] vs. real properties).

I think, that's still valid, and we should keep that. 👍
We added it, as the form on /admin/structure/taxonomy/VOCAB/configure also contains settings for permissions per role and for path alias – which definitely do not belong to the TaxonomyVocabulary instance.

$vocabulary is just a local variable used for taxonomy_vocabulary_save(). No contrib seems affected. I did a quick check on taxonomy_menu and forum (reading code, no actual testing). (And TaxonomyVocabulary:save() discards unknown properties, anyway.)

The i18n module doesn't rely on initially saved config values, but checks form_state in an additional submission handler to switch its specific settings in the vocab config file accordingly.

[argiepiano]

Reviewed and tested LGTM and RTBC.

Thanks for adding the automated test as well, @indigoxela!

As a side comment, unrelated to this PR: it's too bad that vocabularies can't be "extended" easily in Backdrop. In D7, vocabularies were fieldable, or you could even add a column to their table and use hook_schema_alter() to add properties if needed - the controller would take care of saving that new property to its column automatically. I guess the best way for contrib to extend vocabs in Backdrop is to use a separate table or json configuration file to store the extensions, and use the taxonomy_vocabulary hooks to attach the new properties when loading the vocabulary, and save the new properties when saving it.

[indigoxela]

@argiepiano many thanks for reviewing. 👍

Yes, compared to D7 the vocabularies feel a bit reduced in flexibility. But I think, that still meets more than 80% of use-cases. And extending is still possible (i18n_taxonomy does by hook_schema_alter() and grabbing info from the extended config directly).

[quicksketch]

Thanks folks! Great discussion and implementation. I merged backdrop/backdrop#4497 into 1.x and 1.25.x.