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

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.

1. Typehint the form argument if you really want to force validation
2. Surely there's no reason for it to be setting an error in the dispatcher even if you do want to do that type of sanity check
3. My previous point still stands, how in core is the event being dispatched without a valid JForm instance, and if it is indeed possible, why are we not doing this same level of sanity checking against every argument in every event?

@zero-24 I think you broke something here.

@zero-24 I think you broke something here.

Yes thanks should be fixed now.

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.

@zero-24 if this is approved then the same changes will have to be made to the privacy plugin (if it ever gets merged) as it uses the same code

Sure I can do a PR than. Lets get some tests and merge than we can extend that to this plugin too.

I dropped it also in this PR #20275 . So far the tests I did doing this PR work ok.

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