update finished function, issue-27 #78
Conversation
There was a problem hiding this comment.
From your response in #78 (comment) it sounds like when $page is null, that's actually an error state which is unrelated to the issue this PR is trying to resolve.
Nothing about this PR introduces a new scenario where $page is null, so there's no need to add handling for that scenario in this PR. Returning null here is unexpected, and the behaviour of anything calling this method and getting a null result is undefined. So as per below, please remove that.
There was a problem hiding this comment.
Did you test this PR before submitting it?
It doesn't resolve #27 for me. When I test it with a debugger, the finished() method doesn't even get hit before the error occurs.
I'll add a stack trace in a comment to #27 which will show where the exception is actually being thrown from. It seems to stem from ElementalContentControlExtension in dnadesign/silverstripe-elemental, which I note you mentioned you had changed in #27 (comment)
|
I was getting same error which has been mentioned in Issue 27 i.e. I added that and I do have deployed my changes to the prod on our project and I think it's been 3 months now since I have deployed the changes to prod and doesn't seems to cause any issue so far, Also this is only half part of fix, I also had to make changes in silverstripe-elemental in |
Can you please create a pull request which introduces those changes? This PR is not useful without the full fix. |
|
Hi, Sure, just created a new PR for other changes, please let me know if something needs to be changed thanks |
There was a problem hiding this comment.
This is the second half of the fix - silverstripe/silverstripe-elemental#1107 solves the other half.
Looks good to me, thanks for fixing this.
Issue #27