yireo / Yireo_GoogleTagManager2

Open Software License 3.0
148 stars 76 forks source link

Fix issue #208 #209

Closed thomas-kl1 closed 9 months ago

jissereitsma commented 9 months ago

Good stuff. Few comments, though.

First of all, why did you add final to the functions? I've never thought about doing this and I'm not sure whether it has a performance benefit. The only benefit is that it prevents from binding the outer object (closure-principle) or binding things explicitly. But both things are not at hand in this case. Perhaps I'm missing something.

I definitely like the additions of the messages in exceptions.

As for "the loading in the loop" comment, it is not applicable. Using the method addCategoryIds() multiple times actually does not do anything yet, because the method is designed to be called before the actual loading takes place.

So apart from the final question above, all is good to be merged!

thomas-kl1 commented 9 months ago

Not "final" but "static", it's optimization for anonymous function when $this bind is not needed. It's common fine tuning

You may fine more information https://www.php.net/manual/en/functions.anonymous.php or in any blog post that explains it better than I could 😅