Wrong export of context_definitions resulting in Fatal error node form

Import where? Maybe your target site has an old ctools version? I can't imagine this being a bug in the export/import process.

I have the same issue. With a small difference: I have no yml-files, I ex- and import from the database.

We are using the latest release of all modules (Ctools alpha 22 in this case), and are experiencing the same problem. I'm still digging through the code to see what happens, but the symptoms are as follows: another team member exported Pathauto pattern config entities. These include the correct context definitions for the language. However, after having installed the site from scratch and imported all our config into it, these context definitions are empty in the database. This is the only difference between the config before and after the import.

I traced this down to PathautoPattern::preSave() overriding ::$context_definitions, which means the property's values changes during save operations. Why it's emptied, though, I don't know yet.

I think this cleans up ::preSave() a little, as ::getContexts() merges ::$contexts with those from the alias type, and then ::preSave() removes those again anyway.

I apologize for the lack of interdiff, Xano's code is small enough though that I think visual is ok here. I mostly adopted it with one minor change and some added docs explaining why.

The postLoad() code looks totally non-essential in retrospect. I move the spirit of that code into the getContexts() method so that it now properly instantiates contexts for us instead of all contexts always being loaded on load. Also, I moved $this->contexts to default to NULL so that we could tell if it's been instantiated yet. This will later be set to an empty array if no context relationships were generated.

I updated relevant methods to work with the changes to getContexts().

Eclipse

Yes, that goes into the direction I was thinking about.

I'm wondering if we should even go a step further and make context definitions the only API you use to externally to set them up. then we can also avoid the logic in preSave() as the context definitions would be the only relevant definition.

If you want to add a context definition/relationship, you'd use an API to do so, which would invalidate $this->contexts so we re-create them if needed in the same object lifetime.

I'll try that out as I'm interested in seeing how that would look.

Something like this.

I also renamed context definitions to relationships, because that's what it is IMHO. it's not a context definition, just a reference token/thing + label.

Will fix up the documentation around those methods if you like the direction.

A quick scan of the interdiff looks kosher to me.

Eclipse

Ok, this was a bit more complicated than expected to finish ;)

Added documentation.

Then I noticed in applies() that we didn't actually *used* getContexts(). We called it but only to get the relationships as we then again got the contexts from the typed data resolver. That means we actually created the context object at least 4 times until we actually used it. So I changed that to simply call getContexts(). And it stopped working.

It took me a long time to understand that it's because of the cached relationship contexts. Which makes no sense because if we get a different context from the alias type plugin, then obviously we can also get different values for our relationship contexts. And since my refactoring above, we don't ever call getContexts() except in applies().

Which means that we can remove the property and attempt to statically cache that information completely. I also simplified it a bit to avoid re-creating the contexts, we can change the label without that. I also opened #2663002: Support passing in a label to convertTokenToContext() to simplify this code even more.

I also added a try/catch around applyContextMapping() to make sure that even if a relationship gets missing or a condition plugin is misconfigured, we never throw an uncatched exception there that breaks the patch.

Also including an untested update function.

And despite all the added documentation, update function and exception handling, we still end up with fewer lines of code than before :)

Please test this, also updating existing configuration so we can get this in asap.

I tested this on our current project and the import and export work now. This patch adds some detail to docblocks.

The remaining problem is that the format of the relationships property has changed, and its schema (ctools.relationship) no longer applies. Shall we revert the format?

Actually, labels are optional and can be null as well.

The schema was actually valid, but since we do not use the id key anymore, berdir and I thought it best to provide our own schema (for now).

Going to let this sit for a day or so in case Dave or someone else wants to review as it is quite a change. I think it's ready, however.

Ok, that was enough waiting, committed.