Doesn't work. Still getting the error when saving weblinks:
PHP Notice: Undefined property: WeblinksTableWeblink::$newTags in .../administrator/components/com_weblinks/tables/weblink.php on line 138

Also, shouldn't that if code be this?
if (!isset($this->newTags) || !empty($newTag))

What's going on there is kind of tricky because we do not want to do anything if there are no new tags and no old tags. So we don't want to mess up that comparison.

That's not what I mean.
You have:
if ($newTags != array() && !isset($this->newTags))
Meaning the $this->newTags is only set if it isn't defined yet AND in the given $newTags is not empty.

I think $this->newTags should always be set if it isn't defined yet (even if $newTags is an empty array).
ANd it should be overwritten if $newTags is not empty.... right?

Hi, I've updated just for articles. See if that works and then we can fix everywhere else. The issue is that we can't send that into the parent store method for JTable, that's why this whole thing is so tricky and we had the messy workaround.

Isn't this cleaner/DRYer instead of the complete if?

return $result && $this->tagsHelper->postStoreProcess($this, isset($this->newTags) ? $this->newTags : array());

Or even just do this before the return:

$this->newTags = isset($this->newTags) ? $this->newTags : array();
return $result && $this->tagsHelper->postStoreProcess($this, $this->newTags);

And I still think that if in libraries/cms/helper/tags.php is not correct (see my previous comment).

You could do that certainly, and I thought about it, but it's not really about DRY in part it is about readability where people are really copying the same code to their extensions and wanting to understand why. So really a better comment might be to add more comments, which I will assuming that this worked. I'm taking out the change to the other part.
The if there is absolutely correct, try it if you don't believe me, you'll get an sql error.

Definitely add comments in the code to explain what you are doing.

but it's not really about DRY

I disagree. Core code should be about DRY, stable and maintainable code. It's a real pain to have such a high percentage of copy/pasted code everywhere. It is something we are trying to reduce and get rid of.
If you are doing an if/else and the code inside is 90% the same, you are doing it wrong.

I agree that the best approach would be to make sure $this->newTags is always set. I believe this would require a change to JTable, but it would be fine with me. Setting $this->newTags before calling the table's store() method doesn't work, I think because it gets overridden. That's why we need it in JTable (I think). Haven't had time to test this.

You mean like #1409 :)

Actually Mark I'm happy with the code you proposed, except I'd like to see
comments like what I did. Unfortunately we are making JTable into less and
less of a base class and more of what JTableContent/JTableCorecontent
should be since many JTables have nothing to do with content. We can do it
if we want but then let's revisit having a more general table class.

On Sun, Jul 7, 2013 at 6:59 PM, Peter van Westen
notifications@github.comwrote:

You mean like #1409 #1409 :)

—
Reply to this email directly or view it on GitHub#1410 (comment)
.

Actually the more I think about it the more I agree with Peter. People keep asking me why tags is limited to content-like tables, and I think in the end that is probably a mistake.

Closing in favor of #1452