Remove query_cache behavior from codebase

[kasparsatke]

Fixes #95 and removes the query_cache behavior from the codebase.

[mringler]

Great, thank you for the initiative!

I wonder though, should we just remove the behavior, or should we rather disable it? Just removing it leaves users with an unexpected Exception like "query_cache behavior not found".
Shouldn't we rather clear out the code and just have it output a warning, like "The query_cache behavior is not necessary in Perpl and will be removed in future versions. Please remove it from your schema.xml" or so?

[kasparsatke]

Actually I already had some doubts when authoring last week as we recently have been talking about deprecation before removal.

I still PR’ed today partially because I forgot about this since last week, but partially also since in the back of my mind I thought nobody is using this anyways & I have already undocumented it.

But you are right, it would be nice somehow to have some fallback at least in case anyone is actually using this behavior.

[mringler]

I wonder how many people have it enabled without using it. This is one of those behaviors that's quickly enabled and then forgotten.

Not sure what is the best way to disable it:

There is trigger_deprecation() (like here) but I think those messages go into the log, not to the console output (unless -verbose is given).

There is the command OutputInterface, which has println() and supports formatting and such, but I don't think the behavior can access that directly. Building a mechanism into ModelManager is probably a bit much (but let me know if you have an idea about that).

Which leaves simple echo, which I think is fine, too. Usually, echo sucks because you don't know where it comes from, but in this case, that is not a problem.

[kasparsatke]

I have also thought about something like this but any of those three options make no sense in production. If everything works and you aren't curious - chances are you will never see those messages at all.

I could think of something while manually doing composer update but then again if you are doing automatic/scheduled updates the problem is the same again.

I think there is no reliable way to gracefully notify the user while he is in production mode only. Either a hard error or he problably won't see the deprecation.

Also, a deprecation notice alone would not disable something. We would have to actively write some disabling logic. But this would still not close the gap to the hard removal. I mean we could do it of course with a boolean config pref that respects the current environment of the user (behavior in use?) otherwise defaults to disabled for new installations and schema reversals. In the latter occasions, a user might then also even be able to see console warnings.

Saying this, as we had already some new/updated features in the recent past that required config prefs, I predict we also will need to employ some more sophistication about config prefs as at some point in the near future they might become to many to handle them with ease.

The only thing I can further imagine is preparing the user upfront with changelogs and sufficiently long grace periods between deprecation and removal. Still then, one has to read changelogs which might not happen with automatic updates. Best thing one can do is combining the grace periods with semantic versioning.

How do you feel it about this in general (not only this PR) - maybe let's dicuss it in the discussions section?

[mringler]

I would like to keep the door open for people switching from the fading Propel2 to Perpl as long as possible. Meaning I don't see the actual class going away anytime soon.

So if a user does not care about the messages in console, log, or code, well, let them. We probably gain the least from creating exceptions that force them to deal with something they apparently do not want to deal with.

For the changes we put behind config options, I would like to add notifications that are printed when certain cli commands are run, i.e. notifications about optional changes to the model classes could be printed when model:build is run. I imagine this similar to the way npm keeps reminding about a newer version.
But with query_cache behavior, this would only work if we manually check if the behavior is used, which is probably not what we want (we want to remove old code with this, not add more).

For query_cache, I would propose:

1. Let it keep generating the public methods (like getQueryKey()), as removing them might cause needless exceptions in user code, but remove as much of the internal code as possible, particularly the caching functionality. Put a trigger_deprecation() notice in all of them.
2. Add a constructor that also calls trigger_deprecation(). And echo the message to stdout, or check if this can be done with log statements.
3. Update documentation page in case someone stumbles over the messages and wants to find out more - a link to that could be part of the stdout deprecation message. The original discussion or a page in the Wiki could also be used.

Eh voila. Not sure what else we could do. Make the behavior not do what it doesn't do well and try to tell people what they need to do to get rid of it and point them to where they can find out more. This does not have to be pretty, just obvious.

[kasparsatke]

I would like to keep the door open

I fully understand.

This does not have to be pretty, just obvious.

Fully agree.

could be printed when model:build is run.

I get it, my point is it just might not be obvious. For this example, I myself would never see this in a normal setting as I automatically run all this migration stuff and also composer update via Github Action on each commit. I would only see this once for whatever reason I have to do it manually.

Update documentation page

Fair point. Maybe I should not have been so quick with un-documenting. I could put it there again, but this time with a warning or dustbin sign like MDN does like e.g. here: https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/mediaGroup

Or we could hide it in an extra page for deprecated features in the documentation, so it does not look like we actively support those deprecated features (although that contradicts the „make it obvious“ intention a bit).

Anyways, I will try to come up with a new PR along the lines of what you suggested.

[kasparsatke]

For now, I have reverted the documentation for query_cache.