User tests: Successful: 0 Unsuccessful: 0
If the passed parameter is not a valid form our system is broken at all.
Still works as expected
There is a check if there is a valid form. But if at that place is somehow not a valid form the site is broken anyway.
none.
Status | New | ⇒ | Pending |
Category | ⇒ | Front End Plugins |
8851fd3#diff-23e0a7ef508c9ce6eac98c42767a6077
Considering the commit makes no references to a tracker item and the lines in question are part of a much larger change, I'd suggest good luck figuring out the why. At this point I think you're best off going through the code between the form being created and the event being dispatched to see if there's any way the variable would NOT be a JForm object.
I'd say this looks just like a regular sanity check on the passed data.
Which actually makes sense to do since you should never just blindly trust passed arguments.
Labels |
Added:
?
|
Category | Front End Plugins | ⇒ | Administration com_cpanel com_messages Language & Strings Front End com_contact com_content com_mailto Libraries JavaScript External Library Plugins Unit Tests |
Labels |
Added:
?
?
|
Category | Front End Plugins Administration com_cpanel com_messages Language & Strings com_contact com_content com_mailto Libraries JavaScript External Library Unit Tests | ⇒ | Front End Plugins |
It sure can be improved but I don't think removing is the best solution.
Imho setting an error isn't needed. The plugin should just silently refuse the work (return false) instead, maybe log something if you really want.
Typehint means it generates an error, which imho would be even worse.
The only way I could think when that condition is triggered is if a 3rd party calls the event without a valid form. Of course we can question if we want to take measurements for that case.
My understanding is that each code should make sure the data it works with is the expected data. Especially if the data is coming from "outside". So the check itself doesn't seem wrong to me.
Considering the commit makes no references to a tracker item
which might have meant it was part of a security fix?
Considering the commit makes no references to a tracker item
which might have meant it was part of a security fix?
Doubtful. The absolute worst thing that check could've fixed was a path disclosure when a PHP warning/error was triggered.
If we really need this level of checking on plugin arguments, then why is ONLY the form argument in these two listeners checked and not every other argument in every other listener?
Typehint means it generates an error, which imho would be even worse.
If the event listener requires a JForm object and someone doesn't pass a JForm object, then rightfully it should fail. By that argument (an error will be triggered on invalid argument type) we should have duck typing for our entire API and no typehinting at all.
If the event listener requires a JForm object and someone doesn't pass a JForm object, then rightfully it should fail.
Agreed. But is it really the job of a random plugin to do that? I always understood plugins in a way that they should not break the request cycle and be more or less transparent to the caller. Checking the argument so I can avoid possible errors by just returning empty would be one of those ways. But maybe my understanding is wrong.
Sure I can do a PR than. Lets get some tests and merge than we can extend that to this plugin too.
I think this code just predates the time where we were using typechecking across the board. The fields plugins have been using this for a while now and no one has screamed so I'm pretty sure normalising our code on this is just fine
Status | Pending | ⇒ | Ready to Commit |
Labels |
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-05-23 23:53:38 |
Closed_By | ⇒ | mbabker | |
Labels |
Added:
?
Removed: ? ? |
before we accept this i think we need to go back to when this code was first merged to see why it is there - the must have been a reason