You need to change libraries/cms/helper/helper.php too

Thankyou :) Implemented

Works now, added information to the tracker item, thinking about setting it to Ready to Commit, have my doubts that someone has a component/environment to test

Thoughts?

I can adapt the restaurant component tonight hopefully that I had from the life sprint

@dongilbert this is still the same interface as @phproberto 's PR I assume you're still happy with it?

Looks good to me, and mostly BC. The only problem I see is that extending classes that override some of these methods (minus the __construct method) will receive a E_STRICT warning since the method signatures no longer match.

As all these methods already exist in JTable wouldn't they already throw an E_STRICT for not following the parent class signatures?

Note: This statement has been made off the top of my head without fact checking. Feel free to bash me for it :D

Whatever, better an E_STRICT then a fatal error as we have now

I'm not sure what you mean? None of the files modified in this PR extend JTable.

Don't worry I completely misinterpreted what you said. I see what you mean now. I agree with Robert though

I think the discussion is academic, why someone should extent the helper classes that are changed in this PR? And when someone does it is only a warning he gets.

You can try testing the FOF route with this component https://github.com/wilsonge/restaurant-fof - Full instructions on the tracker

I tested on Joomla 3.2.3, using com_reviews and after applying this patch I got:

Error storing tags
Error storing tags

I checked and it's caused by libraries/fof/table/behavior/tags.php:50 (mind the TODO):

/**
 * The event which runs after binding data to the table
 *
 * @param   FOFTable        &$table     The table which calls this event
 * @param   object|array    &$src       The data to bind
 * @param   array           $options    The options of the table
 *
 * @return  boolean  True on success
 */
public function onAfterBind(&$table, &$src, $options = array())
{
    // Bind tags
    if ($table->hasTags())
    {
        if ((!empty($src['tags']) && $src['tags'][0] != ''))
        {
            $table->newTags = $src['tags'];
        }

        // Check if the content type exists, and create it if it does not
        $this->checkContentType($table, $options);

        $tagsTable = clone($table);

        $tagsHelper = new JHelperTags();
        $tagsHelper->typeAlias = $table->getAssetKey();

        // TODO: This little guy here fails because JHelperTags
        // need a JTable object to work, while our is FOFTable
        // Need probably to write our own FOFHelperTags
        // Thank you com_tags
        if (!$tagsHelper->postStoreProcess($tagsTable))
        {
            $table->setError('Error storing tags');
            return false;
        }
    }

    return true;
}

It also gives this error with the latest FOF version.

@jurianeven This is when saving a restaurant right? This is so weird. Mine worked. I'm at my parents over the weekend but will set up a clean install and try and work through it if there's an error somewhere though :/

@nikosdion could you look at the FOF code that was pointed out by Jurian as well please :)

@wilsonge Yes, maybe you're using a different version? It works when I save it without a tag.

The error is thrown by Joomla!'s JHelperTags::postStoreProcess. That's outside FOF, we can't touch it.

Balls. OK will start working on it

Double bugger. I hard coded jos_ in the install.sql that added the content type in that component. Facepalm. @jurianeven can you try again please. I've merged master into this PR as well so it's not so out of date and you'll have to redownload the sample component as well. This worked for me on a clean copy of the branch and a zip of the component so fingers crossed :)

It works with your interface branch and newer com_reviews version :)

Sweet :) sorry about the first test! RTC

No problem, thanks for implementing tags support :)—
Sent from Mailbox for iPhone

On Tue, Mar 25, 2014 at 12:01 PM, George Wilson notifications@github.com
wrote:

Sweet :) sorry about the first test! RTC

Reply to this email directly or view it on GitHub:
#3236 (comment)

Looks like this may have broken:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33708

Without seeing someone's code with the issue, the only way there could be
an error is if their method declaration doesn't match what has been in
JTable, and now the interface, for a little while. Exactly as the message
says. With E_STRICT reporting enabled, they would have seen a similar
message as that is also a strict standards issue. But, strict standards
issues don't stop operation; methods not matching an interface does.

There are two options with this:

• Keep it, it forces developers to bring their code up to standard
• Revert it, which means FOF developers can't use core features which are present to those using just the core code

On Tuesday, May 6, 2014, Nick Savov notifications@github.com wrote:

Looks like this may have broken:

http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33708

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

After some research, Michael is correct. The developer originally extend JTable, which is the right thing to do. However, they chose to ignore the E_STRICT errors that their code was generating due to the non-matching method signature. Once we made created JTableInterface (with the exact same method signatures as JTable), the code broke since the developer was no triggering an E_FATAL.

This is all because they purposely chose to ignore the E_STRICT errors. It's not a B/C issue when a developer doesn't follow E_STRICT and then it bites them in the behind later on.

Fixing the E_STRICT has no impact on existing code while also allowing it to work on future code. That's what they need to do. It's rather unfortunate, but with so many 3rd party developers, it's bound to happen

Thanks guys! I'm not sure that it shouldn't be considered a break in backward compatibility though. If people were previously getting a strict standards error in 3.2, why are they getting a fatal error in 3.3? There would have to be a change in API that broke what was previously generating only a strict standards error.

When an interface is implemented, all child classes of a class implementing
that interface must use the method declaration exactly as specified by the
interface. That is PHP logic out of our control.

The change in API is implementing that interface which is B/C with the
existing method declarations in the base JTable class. An interface does
not allow inconsistencies in method declarations as did just inheriting
from a parent class, so those who had the E_STRICT errors and chose to
ignore them are now dealing with fatal errors.

On Tuesday, May 6, 2014, Nick Savov notifications@github.com wrote:

Thanks guys! I'm not sure that's correct though. If people were previously
getting a strict standards error in 3.2, why are they getting a fatal error
in 3.3? There would have to be a change in API that broke what was
previously generating only a strict standards error.

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

So if you're extending a class and don't follow it you get an E_STRICT. If you don't follow an interface you get an E_FATAL. Point is if you don't follow the API then you get what you deserve really :/

So, it's possible to fix this by making JTableInterface just an empty interface and leaving the changed type hints (where JTable was changed to JTableInterface). I'm not sure how this would effect the FOF users though, and doing it this way makes code completion in an IDE useless, but I'm pretty sure it would work just fine.

FOF defines an empty interface if it isn't present (so older CMS releases).
So while theoretically it "fixes" the issue, we enable developers to
continue pushing code that doesn't adhere to the API.

On Tuesday, May 6, 2014, Don Gilbert notifications@github.com wrote:

So, it's possible to fix this by making JTableInterface just an empty
interface and leaving the changed type hints (where JTable was changed to
JTableInterface). I'm not sure how this would effect the FOF users
though, and doing it this way makes code completion in an IDE useless, but
I'm pretty sure it would work just fine.

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

I totally agree with the sentiment - but is 3.3 the place to enforce that?

I'm not sure if this is helpful or not, but here goes...

In one instance of Joomla 3.3.0 where a user got:
Fatal error: Declaration of store() must be compatible with that of JTableInterface::store()

, if I change the line from:

 function store()

to:

 function store($updateNulls = false)

, the fatal error goes away and it works without issues.

If I run the:

 function store()

in Joomla 3.2.3, it works without issues. Even with strict standards enabled, there is no error/warning message generated in the admin views concerning store().

If I update to Joomla 3.2.4 (not 3.3.0), I get the:
Fatal error: Declaration of store() must be compatible with that of JTableInterface::store()

Unless:
1) the strict standards message is being generated in-between views and thus not showing up in a view in the backed
or:
2) the strict standard message is only being logged in an error log file, rather than displaying in the backend

there appears to be a break in backward compatibility, since strict standards were not being generated for the API, then a fatal error occurred. I doubt (2) is happening, since other (non related) strict standard messages are appearing in the backend for the same views.

My guess is that wherever store() is defined, there's a $updateNulls = false for the parameter, thus an argument isn't needed when calling the function nor is a strict standards message generated as a result.

On Joomla < 3.2.4, if you extend JTable, override the store() method, and fail to put in the $updateNulls = false default argument, then you will get the E_STRICT error.

This also applied to Joomla >= 3.2.4, but since you're talking about this one specific scenario, that's what I focused on.

The fatal error is happening because the developer didn't match the existing API in their extending class, which generated E_STRICT errors. Then, when we switched it to an interface, it now generates an E_ERROR. That's the long and short of it.

This is 100% the developers fault for not following the API to begin with and choosing to ignore the E_STRICT warnings they received (or missed because they didn't have it enabled). However, the question is do we punish them in a minor release for not following the API and "whoops, something broke", or do we stick to our guns and say "If you were following the API correctly in the first place, this wouldn't have happened."

It wasn't generating a e_strict error though, from what I could tell. There were other e_strict errors, but not for that call.

Are you sure you didn't leave the $updateNulls = false in there during your testing for E_STRICT?

This is simply how PHP works. Testing or not - if you don't follow the method signature in an extending class, you get an E_STRICT error.

Yes, positive. It was a clean install (I hadn't tested any modified code on it) that I had lying around. I started it on 3.1.5, then went to 3.2.3, then 3.2.4 without modifying any code.

I'll keep looking for a strict error, but I'm not seeing it at the moment. I suspect that the $updateNulls being set as false (when not defined in the child) is preventing the strict error though.

Since store() is called on a save operation and we have redirects after
actions, it's possible you won't see the E_STRICT error in your browser.
However, if your error_reporting is set to display them in your php.ini
file, they should be getting logged to your PHP error log.

On Tuesday, May 6, 2014, Don Gilbert notifications@github.com wrote:

Are you sure you didn't leave the $updateNulls = false in there during
your testing for E_STRICT?

This is simply how PHP works. Testing or not - if you don't follow the
method signature in an extending class, you get an E_STRICT error.

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

Thanks!

Spot on, guys :)
PHP Strict Standards: Declaration of store() should be compatible with that of JTable::store()

So are we happy to keep things as is with the interface then?

I see valid arguments on both sides. It is technically a BC break, because things that worked fine (even though they generated E_STRICT errors) in 3.2.3 no longer work in 3.2.4. However, the developers whose extensions are having this problem haven't been following the API since 1.6 (if they had, it wouldn't be a problem). This leaves a conflict between adding things and improving the code base vs leaving everything as it is so devs who don't follow E_STRICT can have their cake and eat it too.

TL;DR; it's a tough question. I'm leaning towards making it an empty interface, even though I really don't like that approach.

My view leans towards that that in minor releases if you don't follow the API then you could always potentially run into issues when new features come in. I've only seen two extensions have bugs reported so far and tbh if it's just two extensions I'm kinda happy to leave it as is - they can release updates fast - if it becomes a more widespread problem reported then we can reconsider that decision.

There's only two that we know of.

The PLT will discuss it.

3.3.0 isn't the place to break BC at all. You break BC in major version increments, as stated in Semantic Versioning and the recently adopted Development Strategy.

For what is worth, a minor API change which promotes a Strict notice to a Fatal error is not backwards incompatible. The JTable API did not change. The implementation changed. Properly written code (read: throwing no notices and warnings) still works. Hastily written code breaks. OK, where exactly is the surprise in that...?

The problem lies with the developer who was ignoring the Strict notice in the first place. A Strict notice in the context of this discussion tells the developer loud and clear: "Dude, you are not following the API. Your code is broken, fix it”. The developer chose to ignore the Strict notice and take responsibility for his actions. Why is he coming now hollering that his code broke? It was already broken to begin with! On top of that, you guys published a heck of a lot of betas. Where was he hiding all those months? I think I made my point.

All that said, I personally don’t care if the interface stays or goes. I never liked the way UCM, tags and content versioning are implemented and don’t care much about supporting them in FOF. However, if the PLT plans to remove the interface for the second time (the first one was prior to the release of 3.2.0) please have the courtesy to give us a heads up. That would require a b/c break notice in FOF.

I understand all that, but couldn't disagree more with "Promoting E_STRICT to E_ERROR isn't a BC concern".

If this is a b/c concern then the replacement of JApplication should have NEVER happened in the 3.x series. The external API changed, which is a major b/c break, but the major version wasn’t bumped. Compared to that, the promotion of E_STRICT to E_ERROR is a misdemeanour at worst.

You're forgetting that we just recently published the new Dev Strategy which includes an increased commitment to BC. 3.3 is the first release under this new strategy, and breaking BC in the very first release after we said we weren't going to do that so much doesn't send the best message to the community or give them the warm and fuzzy feeling we want them to have re the project.

Don, the JTableInterface was something we committed during the RAD Code Sprint last year to the then-under-development Joomla! 3.2. For some reason that’s beyond me, someone decided to remove it before the final release. The lack of the interface is a bug that’s been plaguing the entire 3.2 release. Is fixing a bug that shouldn’t be there a b/c issue? Maybe. But if you remove the interface then you’re still causing a major issue in Joomla! 3.

Maybe you can just remove store() from JTableInterface which is TERRIBLE development practice but solves your conundrum: it doesn’t break b/c, it doesn’t maintain the bug which was inadvertently introduced in 3.2.0 but it also doesn’t guarantee that some absent-minded developer will screw up when interfacing with com_tags etc. Seeing where you’re standing right now, if I were you I’d choose this approach, hitting my head hard on the wall while typing git commit -am “Shooting myself in the foot” && git push

Nik, the proposed way to fix it would be to empty JTableInterface and make it a placeholder for 4.0 when we can actually break BC under the current dev strategy.

I guess that’s also a passable solution. The only side-effect is developers touching the core tags / UCM / content versioning code don’t have IDE auto-completetion but they can live with that for 1-2 years until 4.0.

That was the main downside I saw as well, which is not ideal in the slightest. There's good arguments both ways for it. The PLT is discussing how best to solve it. If it just two extensions that can release updates quickly enough for it, then I guess it could be ok. But if that's ok, where does it end? Can we add a JViewLegacyInterface that has the display($tpl = null) method signature and then implement that in JViewLegacy? That does the same thing as this (puts an interface behind an implementation which will probably generate E_ERROR for many extensions).

IMO the "is this BC" check shouldn't be determined by how many extensions it will effect, because that's very subjective and there's absolutely no way of knowing. Maybe we should come up with a list of "things that break BC" and ask for community feedback. I for one would insist that as a general rule, adding an interface to an existing implementation is a BC problem and is something that shouldn't be done. The problem with doing that (adding an interface) is the very thing that is being discussed here - it can turn E_STRICT issues into E_ERROR, and break sites, which is what we want to avoid.

Nic, could you link to the commits where the interface was added and
dropped during 3.2? I'm without internet this week without being somewhere
with free wifi so chasing those isn't so easy for me right now. I
truthfully don't remember the interface being added or removed, so I'd like
to try and at least understand what motivated that (even if it is 6 months
too late).

On Wednesday, May 7, 2014, Nicholas K. Dionysopoulos <
notifications@github.com> wrote:

For what is worth, a minor API change which promotes a Strict notice to a
Fatal error is not backwards incompatible. The JTable API did not change.
The implementation changed. Properly written code (read: throwing no
notices and warnings) still works. Hastily written code breaks. OK, where
exactly is the surprise in that...?

The problem lies with the developer who was ignoring the Strict notice in
the first place. A Strict notice in the context of this discussion tells
the developer loud and clear: "Dude, you are not following the API. Your
code is broken, fix it”. The developer chose to ignore the Strict notice
and take responsibility for his actions. Why is he coming now hollering
that his code broke? It was already broken to begin with! On top of that,
you guys published a heck of a lot of betas. Where was he hiding all those
months? I think I made my point.

All that said, I personally don’t care if the interface stays or goes. I
never liked the way UCM, tags and content versioning are implemented and
don’t care much about supporting them in FOF. However, if the PLT plans to
remove the interface for the second time (the first one was prior to the
release of 3.2.0) please have the courtesy to give us a heads up. That
would require a b/c break notice in FOF.

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

George had found these commits, I think he's the right person to ask. I couldn't track them down myself :)

Ping @wilsonge !!

3.3 is the first release under this new strategy

That's actually not the case. 3.3.0 is released under the old strategy. Otherwise we could have never raised the minimum requirements with it.
3.4 will officially be the first version released under the new strategy. Or also 3.3.1 if you want since it's a bit tied together.

Also the issue is not what you do but why you do it. Fixing a BUG means no breaking BC, implement a feature means breaking BC thus wait for major. BUG in the meaning of programming error, not adding functionality, even when most desired.

Just for clarity, we're actually talking about a potential BC break that effects potentially dozens of extensions, two of which have been reported.

I've seen reports for at least 4 different extensions and 1 potential core issue:
http://forum.joomla.org/viewtopic.php?f=9&t=843861&start=30#p3170305

Though, it's probably from an extension that implements tags.

IMO, raising an E_STRICT to an E_ERROR is hands down a BC break, no questions asked no matter which way you slice it. Now, the question remains of how the PLT will handle the situation.

And it seems long time PHP devs whom I respect agree with that assessment: https://twitter.com/dilbert4life/status/464137613637459968

Agree with Don

Well PLT to decide. But my vote would be to keep it in 3.3 and remove it from 3.2.4 - that's what a minor is for in my personal opinion. If people don't follow API risk is there - just a low one for poor extension practice.

RE: The interface missing from 3.2 - I traced the commit of the interface (please don't ask me where again - it took over an hour to find it) - but for some reason it vanished and never made it into the actual PR that included FOF - so my assumption is that it got removed when rebasing it on master at some point.

Sorry George, I'm having trouble understanding what you mean. What is a minor version for in your personal opinion?

New features - if extensions don't follow the API and a new feature screws them over then it is what it is. For example, say someone already has a class PasswordHash in a 3PD extension for Joomla - now by you guys bringing in https://github.com/joomla/joomla-cms/blob/staging/libraries/phpass/PasswordHash.php (I know this class was bought in before 3.3 - but it's just Proof of Concept) technically it's a B/C break. Same if someone already has an extension com_tags installed and core introduced com_tags. But you'll still release it - even if you screw that extension around bit. I call it as being exactly the same thing here.

What I would say is that the interface I guess was a feature and probably shouldn't have gone in 3.2.4.

Of course as I say this is my personal opinion :)

If I'm reading this correctly, https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/table/observer/tags.php#L108 will also result in a fatal error.

Reference:
https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/helper/tags.php#L811

No because JTable implements JTableInterface - so there won't be any issue. Note before that was typehinted with JTable - which means any table doing tags MUST have extended JTable and therefore as long as they didn't have E_STRICT's will be implementing JTableInterface automatically without any need for any code change

Btw actually I found the original stuff from the mailing list. Roberto's PR to implement it is here joomla-projects#95 and was merged into the projects repo. But for some reason that never made it into here #1911 (the PR from the projects repo to the main CMS)

Sounds like the interface not merging was a silly mistake then. Sucks it
didn't get merged back then, but I'm a bit relieved that it wasn't pulled
out after the fact.

On Wednesday, May 7, 2014, George Wilson notifications@github.com wrote:

Btw actually I found the original stuff from the mailing list. Roberto's
PR to implement it is here joomla-projects#95joomla-projects#95and was merged into the projects repo. But for some reason that never made
it into here #1911 #1911 (the
PR from the projects repo to the main CMS)

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