DanielSiepmann / tracking

TYPO3 Extension providing server side tracking.
https://daniel-siepmann.de/projects/typo3-extension-tracking.html
GNU General Public License v2.0
17 stars 2 forks source link

Error while copying page #52

Closed tritum closed 3 years ago

tritum commented 3 years ago

Steps to reproduce:

Error message (example):

Attempt to insert record "tx_tracking_pageview:88" on a page (708) that can't store record type

Tested with a brand new and clean v10.

Thanks for this great extension and taking care.

All the best, Björn

DanielSiepmann commented 3 years ago

Thanks for reporting. I'm not aware of a simple technical solution.

I would say you don't want those records to be copied. Also, you don't want to show the type of records when creating new records. Therefore, they are not allowed on standard pages. Unfortunately that leads to the above error message. TYPO3 does not offer an easy way to exclude tables from copying.

Integrators can setup tables_modify to exclude that table. But that doesn't work for admins. Admins always have full access to everything. There is also no hook available to modify that list.

I probably would stick to current solution and add a hint to the documentation why this message is shown and that its expected. What do you think?

tritum commented 3 years ago

Thanks Daniel for your fast reply and useful explanation. The problem is that the core has moved a lot of processes to AJAX. The AJAX integration seams to be a little too picky. IMHO the AJAX process dies as soon as an error (like the one described above ) occurs. This is not only happening when copying pages but also while deleting pages. As a result I cannot delete pages anymore.

Maybe we should change the AJAX process in the core to handle such errors differently. Or the process itself should not trigger an error but a warning wich in turn does not kill the AJAX process.

What do you think?

DanielSiepmann commented 3 years ago

Thanks, wasn't aware of that situation.

I won't change TYPO3 handling in that case. It is an error which can be ignored in that case, but usually you would expect those records to be copied. I would propose to add a new ctrl entry preventCopy which would ignore the table on copy operations and also prevent rendering of copy options.

In the meantime: I could allow the record types on standard pages, and hide them on copy. That should work, but be hacky as well. I would add a cleanup command. That would then delete hidden entries. That one could be extended to also remove old entries in future updates.

What do you think?

tritum commented 3 years ago

Hey. You are fast mate!

I have no special preference but the preventCopy control sounds good to me. And even cleaner IMHO.

Copying the data and then hiding it seams fishy. Especially if it results in having a cleanup task/ command. People will forget to use/ fire it. Therefore, preventCopy is IMHO easier and requires no further configuration, does it?

DanielSiepmann commented 3 years ago

Sure, but requires a new feature to be merged into TYPO3, those not available for 10 and lower, only 11 and up. Also, I would need to implement the feature and provide a patch that will need to get merged.

I'll go with the hide for now and try to implement and release it asap.

tritum commented 3 years ago

Okay, now I get it. Sry, wasn't aware of the problem :) Another reason to use v11 ASAP!

I will review any patch. Just let me know.

Thanks for taking care! Björn

DanielSiepmann commented 3 years ago

Finally, found time to give it a try. Can you check whether https://github.com/DanielSiepmann/tracking/pull/55 works? You can fetch the patch via https://patch-diff.githubusercontent.com/raw/DanielSiepmann/tracking/pull/55.patch, e.g. applied via https://packagist.org/packages/cweagans/composer-patches :)

It will prevent TYPO3 from copying tx_tracking_ tables via Hook. A test is added which ensures copying a page works as expected. The hook just slightly modifies DataHandler and shouldn't break anything.

tritum commented 3 years ago

Thanks @DanielSiepmann. Tested your patch. Works like a charm. No warnings/ errors are displayed. During our test we couldn't identify any problems/ side effects. Very well done!

DanielSiepmann commented 3 years ago

Thanks. Released 1.1.1 with the fix.