$this->ptable not available in the DataContainer class

[leofeyer]

In #2039, we have changed the invalidateCacheTags() method, which now accesses $this->ptable. However, protected $ptable is only declared in the DC_Table class and not in the DataContainer class.

[Toflar]

Ah, there are data containers without a ptable. Maybe we should rather check for that in the invalidateCacheTags()? If there's no ptable available, there are also no such tags to invalidate :)

[m-vo]

Shouldn't the invalidateCacheTags method rather live in DC_Table?

[leofeyer]

Shouldn't the invalidateCacheTags method rather live in DC_Table?

That is what I thought as well, however, there might be custom data containers that need the method. On the other hand, these custom data containers will probably extend from DC_Table. 🤷🏻‍♂️

@Toflar So I move the invalidateCacheTags() to the DC_Table class?

[leofeyer]

I have implemented another fix in 76a01b1.

[aschempp]

This will only purge one level up, right? It won't purge tl_page for content elements?

[leofeyer]

Good point! That is another issue indeed (not related to this change though).

[leofeyer]

I guess we have not been traversing through all parent records, because $this->activeRecord only has the current record and the ID of the parent record. And since the method is in the DataContainer class, we would have to overwrite it in each child class, because only the child classes "know" how to retrieve the parent records.

[Toflar]

No, the parent of the parent is not required to be loaded. This has been discussed before and it's all good the way it is, you just need to pass on the correct tags when tagging.
I don't like this implementation here though. Imho we should check for $dc->table and $dc->activeRecord->pid not the globals.

[leofeyer]

Imho we should check for $dc->table and $dc->activeRecord->pid not the globals.

That is not really possible (see the initial issue description). Unless you want to use property_exists($this, 'ptable')?

[Toflar]

I think overriding makes sense but imho the callback should be in the DataContainer and in DC_Table we should only add the ptable-pid thing, the rest should be in parent.

[leofeyer]

I still think it is wrong to have this method in the DataContainer class, because the functionality works with DC_Table only. It would even generate wrong tags if called on a DC_Folder object.

Can we not move the method to the DC_Table class and deprecate the original method in the DataContainer class?

[Toflar]

Can we not move the method to the DC_Table class and deprecate the original method in the DataContainer class?

If it doesn't work we can move it alltogether without a deprecation message.

[leofeyer]

Moved in 115ecca.

[Toflar]

Removing the pid handling is correct because it's not necessary anyway. You should make sure as a developer to tag your response with all the used resources.

[ausi]

Removing the pid handling is correct because it's not necessary anyway. You should make sure as a developer to tag your response with all the used resources.

But for new resources this is not possible as you cannot tag the news archive module with a cache-tag of a not-yet-existing news article. So IMO we have to make sure that creating new elements in a child table actually clears the cache for the parent table (one level up is enought here IMO). For modifications and deletions this should not be necessary I think.

[leofeyer]

But as long as there is no news article, there will be no cached news reader page, either, will there?

[Toflar]

But for new resources this is not possible as you cannot tag the news archive module with a cache-tag of a not-yet-existing news article.

The idea would be to tag all elements working with news with the general contao.db.tl_news and invalidate that when creating a new news entry. Not sure that's the case now but that's the idea.

[leofeyer]

I think this is the case:

[Toflar]

Yeah we're invalidating the specific tag and the general one on every edit which kinda renders the specific one useless...

[Toflar]

I mean, it's okay imho if you don't tag the front end list element for example with contao.db.tl_news which you need if you want to invalidate yourself when there's a new news being saved...

[leofeyer]

Can we improve this somehow? E.g. by not invalidating the general tag?

[Toflar]

Not sure. Invalidating the general tag is kind of like "hey, something regarding the news changed" and maybe you want your response to be invalidated in that case because you don't know. Then we'd need a specific create tag though. But there might be many more. I think this definitely needs an up for discussion label.

[ausi]

The news list module currently gets tagged with contao.db.tl_news.ID for all shown news articles and contao.db.tl_news_archive.ID for the news archives. So we need to invalidate the parent table IMO to make this work.

[leofeyer]

Why? The cache entry for the news list module will be purged if the contao.db.tl_news.ID tag is invalidated.

[ausi]

If you add a new news article the list module on the website will not be tagged with this new contao.db.tl_news.ID and therefore the new article would not show up. If we invalidate the parent contao.db.tl_news_archive.ID (as we did before this PR) the news list module would show the new article as soon as it was published.

[Toflar]

But that's only "by chance". It only works if there is a parent table. If there's not, it wouldn't work. We need special handling for "create".