Implemented tagging responses with comment cache tags

[Toflar]

Implementation for #135

[leofeyer]

I'm not sure if I like adding a new callback. @contao/developers Is this still how we do it nowadays?

[Toflar]

I don't like adding an event here. It's still old fashioned DCA stuff. If we have a new driver, then we might go a different direction.

[aschempp]

I'm generally fine with the callback principle. The markup also works with our new callback service tags.
However, the callback is strange, because it does not pass the datacontainer object, and (almost?) all other callbacks do that. The callback is also missing some arguments (ptable stuff?) and I would expect to return the tags array (or actually return the tags I want to have merged into the array).

[Toflar]

There is no DC, you don't need one. You also don't need anything else (ptable). I could adjust the return value but it makes no sense to me.

[aschempp]

I assume you're right, but it's just kinda inconsistent with most other callbacks. I only question if this is clearly backend related? So is it only supposed to set/clear tags on backend operations? Then it totally makes sense in the DCA. But what if we need to clear tags if the member data is edited in the front end?

[Toflar]

Modify the methods so they actually do get a DataContainer instance and see if I can keep it BC.

[leofeyer]

As discussed in Mumble on November 22nd:

Modify the methods so they actually do get a DataContainer instance and see if I can keep it BC.

[Toflar]

Okay, so BC is impossible but tbh I don't think anyone has already overridden one of the methods in their drivers. Maybe I should make them private anyway, now that we have a callback?
The callback now feels a lot more natural because it gets a dc and the existing tags and expects the modified tags in return. It shows that it really is a callback :)
However, I don't know how to implement it in the reviseTable method. I noticed that there's also no ondelete_callback triggered there. Shouldn't we generate a $dc with all the basic values (id, table, ptable, and activeRecord) and also trigger the ondelete_callback there? If so, I could also call invalidateTags($dc) just the same way then. @contao/developers

[leofeyer]

The easiest solution would probably be to execute

if (System::getContainer()->has('fos_http_cache.cache_manager'))
{
	$cacheManager = System::getContainer()->get('fos_http_cache.cache_manager');
	$cacheManager->invalidateTags($this->getCacheTags($this->strTable, $ids));
}

in the reviseTable() method.

[Toflar]

getCacheTags() now wants a DataContainer.

[leofeyer]

Bummer. Then I guess we have to create a data container object. Or maybe we were wrong about the changes in the first place?

[Toflar]

As discussed in Mumble: Generate a DC for every ID that's going to be deleted and pass this to the invalidation method before the record is actually deleted.

[Toflar]

Done, ready to review 😄

[leofeyer]

After this PR has been merged, we should also fix existing $dc->id calls (e.g. here).

[leofeyer]

Now that this method is no longer used anywhere else, we might as well remove it and execute the logic here. Then it would also become more obvious how the callback should be named: oninvalidate_cache_tags_callback (similar to onrestore_version_callback)

What do you think?

[Toflar]

Nice input! You're right, changed right away: f183f3a

[leofeyer]

One last thing regarding this change:

			case 'id':
				$this->intId = $varValue;
				break;

Could there be someone out there who relied on $dc->id not changing $dc->intId? Then it would be a BC break for them. On the other hand I don't think this use-case has ever been intended.

[Toflar]

I think it was not. Would be very strange to have different id's there.

[leofeyer]

Thank you @Toflar.