Page MenuHomeIn-Portal Phabricator

INP-1381 - Create special template for fatal error page
Needs ReviewPublic

Authored by glebs on Sep 23 2014, 12:25 PM.

Details

Reviewers
alex
Test Plan

Advanced theme should be used. Debug - disabled. Theme files should be refreshed in system tools after applying patch.

  1. Insert <inp2:m_UnknownTag/> into themes/advanced/platform/login/register.tpl
  2. Go to frontend homepage. Click on My Account
  3. You should be redirected to error_fatal template

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
glebs added a comment.Sep 25 2014, 2:20 PM

fixes by Alex's review

alex accepted this revision.Sep 26 2014, 3:01 AM
alex edited edge metadata.
This revision is now accepted and ready to land.Sep 26 2014, 3:01 AM
alex requested changes to this revision.Oct 3 2014, 6:38 AM
alex edited edge metadata.

After using it a bit I've one edge case, that wasn't covered:

  1. try calling method that doesn't exist on an object from anywhere on a page
  2. you'll get non-catchable fatal error back
  3. due that fact we can't nicely get it from our handler and instead we only get it thanks to kLogger::catchLastError method, which is registered via register_shutdown_function
  4. at that point whole application is shutdown already and calling Redirect method won't do us any good

For such cases I propose to revert to old way of displays error message back to the user.

This revision now requires changes to proceed.Oct 3 2014, 6:38 AM
alex added inline comments.Oct 13 2014, 4:36 AM
core/install/upgrades.sql
3008–3010

Several more issues with this:

  1. If a variable is renamed in upgrades.sql then it have no effect on clean In-Portal installation. To make clean installation work it needs to be renamed in INSERT INTO sql in the install_data.sql file as well.
  2. You have renamed variable only, but not it's usages. I've counted 2 usages at least in the Core module.
alex added inline comments.Oct 14 2014, 9:19 AM
core/install/upgrades.sql
3013

The DisplayOrder column value (the 10.04 in above SQL) should be unique across same configuration group. Right now it exactly matches value of NoPermissionTemplate system setting.

You need to:

  1. shift down all configuration variables in same sub-section
  2. use 10.05 as DisplayOrder column value for that new system setting
  3. update install_data.sql to reflect new value in DisplayOrder column

I usually use SQL similar to this to shift up/down system settings in particular sub-group:

UPDATE SystemSettings
SET DisplayOrder = ROUND(DisplayOrder + 0.01, 2)
WHERE Section = 'x' and Heading = 'y' AND DisplayOrder > z
alex added inline comments.Oct 14 2014, 9:22 AM
core/install/upgrades.sql
3013

Just noticed that used label doesn't match system setting name and should be la_config_ FatalErrorTemplate. You can see on screenshot above that two system settings have identical translation.

alex added inline comments.Oct 14 2014, 11:00 AM
core/kernel/utility/logger.php
1161–1162

This code will be used when it's Front-End and Debug Mode is Turned Off. In all other cases please use original error reporting code.

glebs updated this revision to Diff 27.Oct 23 2014, 4:04 AM
glebs edited edge metadata.

fix latest issues

alex requested changes to this revision.Oct 30 2014, 6:14 AM
alex edited edge metadata.
alex added inline comments.
core/install/install_data.sql
48

Wrapping 10.11 in single quotes isn't required. It made sense for 10.10 at some point though.

core/install/upgrades.sql
13

Please don't list column names in INSERT statements, unless it's absolutely necessary.

core/kernel/utility/logger.php
6

This doesn't cover following cases:

  1. non-catchable fatal error (e.g. calling method on a non-object variable, calling non-existing method on an object) described in http://qa.in-portal.org/D3?vs=19&id=27#188
  2. CLI usage

More info:

  1. when non-catchable fatal error happens then $this->Application will most likely reference a non-object (need to debug to be sure)
  2. in CLI (PHP_SAPI == 'cli') the debug mode is turned off (because there is no IP to match debug mode permission by) and it's a Front-End (e.g. /tools/run_event.php or /tools/cron.php)

So I actually recommend checking conditions for old error display way (via DIV) first and none of them matches, then do a redirect.

This revision now requires changes to proceed.Oct 30 2014, 6:14 AM
glebs updated this revision to Diff 53.Nov 4 2014, 6:08 AM
glebs edited edge metadata.

Handle Fatal error in CLI and non-catchable fatal error

alex requested changes to this revision.Nov 19 2014, 12:57 PM
alex edited edge metadata.
alex added inline comments.
core/kernel/utility/logger.php
1155

Here is what needs to be done:

  1. add applicationShutdown property to kLogger class, which default to false
  2. from kLogger::catchLastError method set kLogger::applicationShutdown to true
  3. replace the !$this->Application check with $this->applicationShutdown check

Here is how I was able to the kLogger::catchLastError method:

  1. disable debugger
  2. add $class = new MissingClass(); on top of kMainTagProcessor::Phrase method
  3. go to http://your-in-portal-install.com/directory.html
This revision now requires changes to proceed.Nov 19 2014, 12:57 PM
alex added a comment.Nov 23 2014, 3:10 AM

Adding comment to create corresponding "Issue Link" from JIRA Issue back to this Differential Revision.

alex retitled this revision from INP-1381 Create special template for fatal error page to INP-1381 - Create special template for fatal error page.Dec 7 2014, 3:28 PM
alex edited edge metadata.
alex set the repository for this revision to rINP In-Portal.Dec 7 2014, 3:43 PM
alex added inline comments.Jan 15 2015, 4:38 PM
core/kernel/utility/logger.php
1155

I think we can benefit from using error_fatal.tpl in Admin Console as well, when debug mode is turned off. To make this happen please replace

  • this: $this->Application->isDebugMode() || $this->Application->isAdmin
  • with this: $this->Application->isDebugMode(false)
alex added inline comments.Jan 15 2015, 5:08 PM
core/admin_templates/error_fatal.tpl
47

Please replace text in <strong> tags with la_text_FatalError (also add this phrase to the /core/install/english.lang file).

49–52
  1. Only show error message text when we're in Debug Mode.
  2. Remove error message from session in any case.
core/kernel/utility/logger.php
1162

When we're in admin then use error_fatal as error template and don't read it from system setting to allow Front-End having different template without affecting Admin Console where template name is always the same.

glebs updated this revision to Diff 95.Jan 16 2015, 4:41 AM
glebs edited edge metadata.

QA fixes

glebs updated this revision to Diff 96.Jan 16 2015, 6:29 AM
glebs edited edge metadata.
glebs removed rINP In-Portal as the repository for this revision.

QA fixes

alex requested changes to this revision.Jan 16 2015, 8:31 AM
alex edited edge metadata.

Almost there. Still I think following can be improved.

core/kernel/utility/logger.php
1163–1183

This concatenation style doesn't look right. Please to do this instead:

  1. create $css_properties variable, which will be an array of all selectors (e.g. background-color: #FEFFBF, margin: ' . $margin) with no trailing ;
  2. replace $style variable with implode('; ', $css_properties) . '; construct
1175–1183

Try rearranging code like so:

  1. move StoreVar statement on top of outer else
  2. do ->Redirect call in place (if/else) instead of using helper $error_template variable
This revision now requires changes to proceed.Jan 16 2015, 8:31 AM
glebs updated this revision to Diff 98.Jan 20 2015, 2:47 AM
glebs edited edge metadata.
alex requested changes to this revision.Jan 20 2015, 3:10 AM
alex edited edge metadata.

Just this one minor fix and we're done I guess.

core/kernel/utility/logger.php
27

Inline variable because now it won't create any extra long lines.

This revision now requires changes to proceed.Jan 20 2015, 3:10 AM
glebs updated this revision to Diff 99.Jan 20 2015, 3:15 AM
glebs edited edge metadata.
glebs updated this revision to Diff 100.Jan 20 2015, 3:22 AM
glebs edited edge metadata.
alex added inline comments.Jan 20 2015, 3:25 AM
core/kernel/utility/logger.php
29

The ternary operator must be bracketed because otherwise the 'margin: ' . $this->Application->isAdmin will become it's condition.

alex requested changes to this revision.Jan 20 2015, 3:56 AM
alex edited edge metadata.

The translation of new setting is missing:

This revision now requires changes to proceed.Jan 20 2015, 3:56 AM
alex added inline comments.Jan 20 2015, 4:08 AM
themes/advanced/error_fatal.tpl
20

This looks awful compared to other error pages in theme (e.g. Error Not Found or No Permission).

How this looks:

At least how it should look:

Key missing elements:

  1. navigation bar
  2. heading
  3. paddings
  4. phrases

Maybe something else is missing as well.

alex added inline comments.Jan 20 2015, 4:16 AM
core/admin_templates/error_fatal.tpl
5

The no_permissions title preset is used resulting in No Permissions page title instead of correct one:

glebs updated this revision to Diff 101.Jan 22 2015, 2:54 AM
glebs edited edge metadata.

Fix error templates

alex added inline comments.Jan 23 2015, 8:04 AM
core/units/admin/admin_config.php
17

You shouldn't do any massive unrelated changes unless arc lint complains about them. So please revert.

alex requested changes to this revision.May 19 2015, 7:20 PM
alex edited edge metadata.
alex added inline comments.
core/kernel/utility/logger.php
219

Can't apply patch due conflict with current file version. Please sort of re-base.

1163

Please remove the isDebugMode part (let's use nice template in debug mode too).

core/units/admin/admin_config.php
17

Not done.

themes/simple/_install/english.lang
307

Please revert, because this change isn't related this this task.

309

Please revert, because this change isn't related this this task.

This revision now requires changes to proceed.May 19 2015, 7:20 PM
alex added a comment.May 19 2015, 7:26 PM

Please also write a test plan.

alex added inline comments.May 19 2015, 7:29 PM
core/kernel/utility/logger.php
1163

Actually don't change that line, because we're only getting to _displayFatalError method in debug mode, when debug report is invisible and this happens during ajax request only in which case I want to see failed ajax request instead of redirect.

alex added inline comments.Sep 9 2015, 5:06 AM
core/kernel/utility/logger.php
1177–1182

Please also do this (for debug mode only):

  1. add the system-log_id parameter to redirect params, that would contain ID of just created log record
  2. on the error template use <inp2:system-log_PrintBacktrace/> tag to display expandable stack trace of the error
glebs edited the test plan for this revision. (Show Details)Sep 17 2015, 2:37 AM
glebs edited edge metadata.
glebs edited the test plan for this revision. (Show Details)Sep 17 2015, 2:41 AM
glebs updated this revision to Diff 307.Sep 17 2015, 2:41 AM
glebs edited edge metadata.

Update according last request

alex requested changes to this revision.Nov 13 2015, 4:43 AM
alex edited edge metadata.
alex added inline comments.
core/kernel/utility/logger.php
1169–1176

When in CLI:

  1. don't append HTML/CSS to the error message
  2. also show stack trace by using debug_print_backtrace function

Idea is to have same error display in CLI as PHP builds it, when it encounters unhandled exception:

PHP Fatal error:  Uncaught exception 'Exception' with message 'IntuitAnywhere login failed' in /path/to/project/modules/custom/units/helpers/quickbooks/QBConnectorHelper.php:91
Stack trace:
#0 /path/to/project/core/kernel/utility/factory.php(118): QBConnectorHelper->Init('QBConnectorHelp...', '', Array)
#1 /path/to/project/core/kernel/application.php(2492): kFactory->getObject('QBConnectorHelp...', NULL, Array)
#2 /path/to/project/modules/custom/units/helpers/quick_books_helper.php(296): kApplication->recallObject('QBConnectorHelp...')
#3 /path/to/project/modules/custom/units/organizations/organization_eh.php(1328): IntechnicQuickBooksHelper->synchronizeVendor(Array)
#4 /path/to/project/modules/custom/units/organizations/organization_eh.php(201): OrganizationEventHandler->synchronizeSuppliers(1447405965)
#5 /path/to/project/core/kernel/event_handler.php(116): OrganizationEventHandler->OnQuickBooksSynchronize(Object(kEvent))
#6 /mn in /path/to/project/modules/custom/units/helpers/quickbooks/QBConnectorHelper.php on line 91
This revision now requires changes to proceed.Nov 13 2015, 4:43 AM
alex added a comment.Nov 14 2015, 8:35 AM

Just send revision for review somehow without making any code changes.

core/kernel/utility/logger.php
1169–1176

Please don't do it, because we have separate discussion about this at http://community.in-portal.org/x/ZAD3.

glebs requested a review of this revision.Nov 16 2015, 3:14 AM
glebs edited edge metadata.
alex edited edge metadata.Mar 26 2016, 6:45 AM
alex added a project: Restricted Project.