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 ↗(On Diff #19)

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 ↗(On Diff #19)

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 ↗(On Diff #19)

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 ↗(On Diff #19)

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 ↗(On Diff #19)

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 ↗(On Diff #19)

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 ↗(On Diff #19)

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

49–52 ↗(On Diff #19)
  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 ↗(On Diff #19)

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
1165–1166 ↗(On Diff #19)

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
1171–1179 ↗(On Diff #19)

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 ↗(On Diff #19)

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 ↗(On Diff #19)

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 ↗(On Diff #19)

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 ↗(On Diff #19)

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 ↗(On Diff #101)

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 ↗(On Diff #19)

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

1163 ↗(On Diff #19)

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

core/units/admin/admin_config.php
17 ↗(On Diff #101)

Not done.

themes/simple/_install/english.lang
307 ↗(On Diff #101)

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

309 ↗(On Diff #101)

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 ↗(On Diff #19)

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 ↗(On Diff #19)

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 ↗(On Diff #19)

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 ↗(On Diff #19)

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.