Bug 328971

Comment 1 Remy Suen 2010-10-28 15:48:14 EDT

Created attachment 181988 [details]
Workbench patch v1

(In reply to comment #0)
> Using Command's execute(ExecutionEvent) or executeWithChecks(ExecutionEvent)
> method currently doesn't work in 4.x.

There has been at least two reports from adopters about this incident, see bug 321263 and bug 328953.

Comment 2 Remy Suen 2010-11-29 11:55:32 EST

Created attachment 184055 [details]
WorkbenchPage patch v2

v1 has gone stale.

Comment 3 Paul Webster 2011-06-08 10:13:51 EDT

Created attachment 197602 [details]
Workbench patch v03

Delegate to the EHandlerService.

PW

Comment 4 Paul Webster 2011-06-09 11:28:30 EDT

Created attachment 197700 [details]
Workbench page v04

Delegate to the handler service with the application context that was passed in.

Because by this point in 3.x it has passed other checks, we simply turn into a NO-OP in the exception case.

PW

Comment 5 Paul Webster 2011-06-09 11:29:08 EDT

Remy, could you review this?

PW

Comment 6 Remy Suen 2011-06-09 12:27:44 EDT

(In reply to comment #4)
> Created attachment 197700 [details]
> Workbench page v04

I think we should leave the logging statement to catch offenders of this code pattern. I would also feel safer if we added a check to prevent an infinite loop.

Comment 7 Paul Webster 2011-06-09 12:59:28 EDT

Created attachment 197707 [details]
Workbench patch v05

This adds logging back to catch execute calls, and also adds a check so you cannot feed this handler back into the system.

PW

Comment 8 Paul Webster 2011-06-09 13:01:12 EDT

OK, I've updated to address some comments.

PW

Comment 9 Remy Suen 2011-06-09 13:12:48 EDT

(In reply to comment #7)
> Created attachment 197707 [details]
> Workbench patch v05

Paul, we return 'null' in the LegacyHandlerService if someone feeds MHG into the method. The IHS's activateHandler(*) methods don't specify that 'null' can be returned, they mention returning "a token". Do you think we should instead throw an exception or...?

Comment 10 Paul Webster 2011-06-09 13:32:30 EDT

Created attachment 197711 [details]
Workbench patch v06

OK, we return a bogus activation that also returns correct non-null values where necessary.

PW

Comment 11 Remy Suen 2011-06-09 13:40:23 EDT

(In reply to comment #10)
> Created attachment 197711 [details]
> Workbench patch v06

Thanks for iterating over this, Paul. Looks good to me, +1.

Comment 12 Paul Webster 2011-06-09 15:23:57 EDT

McQ, we're considering this for 4.1.

PW

Comment 13 Mike Wilson 2011-06-09 15:58:45 EDT

getBogusActivation is a bogus name. ;-)

+1, given an updated name and a comment that explains what that method is trying to do.

Comment 14 Paul Webster 2011-06-09 16:18:25 EDT

Renamed to getSystemActivation(*) and released to HEAD
PW

Comment 15 Paul Webster 2011-06-10 08:18:27 EDT

*** Bug 348584 has been marked as a duplicate of this bug. ***