-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[4.0] Replace default symfony error page with a shiny new Joomla one #30056
Conversation
templates/system/fatal.php
Outdated
<meta name="robots" content="noindex, nofollow"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
<title>An Error Occurred: <?php echo $statusText; ?></title> | ||
<style>body { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awkward in the sense that you're copy/pasting all this css. It can be done in a much more maintainable fashion, egg:
<style>
<?php echo @file_get_conents(__DIR__ . '/../css/incompatible.css'); ?>
</style>
fwiw it will need a minor tweak so the css is copied from the build/...
folder to the system template
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember anymore the code that is there but if it is using CSS Custom properties all you have to do is jus redeclare them after echoing the css content
I like it but am going to wait for the 5.2 tagged version of symfony error handler to be out first! I don't think that will be any sort of issue. |
This comment was marked as abuse.
This comment was marked as abuse.
I don't have one yet. It may not cut in time. There's a loose aim to get a release out this year. But currently still at least a few more beta's I suspect before we're going to be able to cut a RC |
This comment was marked as abuse.
This comment was marked as abuse.
I'd be decently confident this will make it before we tag RC for what it's worth and where release blockers are right now |
Note to others: do not test until it is declared ready to test. Otherwise just applying the patch causes a fatal error. To fix: remove the line added in diff libraries/bootstrap.php - line 57. Then revert the patch. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30056. |
This comment was marked as abuse.
This comment was marked as abuse.
@PhilETaylor Looks like it's out https://symfony.com/blog/symfony-5-2-0-released - so whenever you're ready to get that in we can get this tested and in |
@PhilETaylor quick bump |
@PhilETaylor since I saved your sunday could you finish this PR please? |
This comment was marked as abuse.
This comment was marked as abuse.
I would start with upgrading the branch, then changed composer dependencies to ^5.2 and have a look at the css yes ;-) |
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
No drone failure is sadly currently correct. They added a dependency here beberlei/assert#286 in the package your updating to 3.3.0. It got removed here beberlei/assert#289 in their 3.2.5 version. But as per beberlei/assert#309 it never made it into the 3.3 branch :( |
This comment was marked as abuse.
This comment was marked as abuse.
Because you have the intl dependency on your macbook i think. But that's not a required Joomla dependency. It's not even a dependency for their library (aside from two documented functions - hence the re-removal). I've done a PR to their upstream to fix the issue. But remains on them to merge. beberlei/assert#310 |
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
@PhilETaylor so I redid that PR as there was a major refactor of the build tools few days ago and there were a lot of conflicts. // Joomla supplied fatal error page
if (file_exists(__DIR__ . '/../../templates/system/fatal-error.html'))
{
$template = file_get_contents( __DIR__ . '/../../templates/system/fatal-error.html');
} BTW you have also conflicts in this PR |
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
great news |
@PhilETaylor run |
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
file headers and explinations updating composer.lock for ci to work cs remove unused css add Customised Fatal Error Template to .gitignore Pin symfony/error-handler to ^5.2 Signed-off-by: Phil E. Taylor <phil@phil-taylor.com> use fatal-error.html implemented in #31743 Signed-off-by: Phil E. Taylor <phil@phil-taylor.com> Commit new composer.lock Signed-off-by: Phil E. Taylor <phil@phil-taylor.com> add all three placeholders for completeness Signed-off-by: Phil E. Taylor <phil@phil-taylor.com> Update templates/system/fatal.php Co-authored-by: Dimitris Grammatikogiannis <d.grammatiko@gmail.com> Update templates/system/fatal.php Co-authored-by: Dimitris Grammatikogiannis <d.grammatiko@gmail.com> Chane paths, custom filename, and order od loading Signed-off-by: Phil E. Taylor <phil@phil-taylor.com> composer req symfony/error-handler merge gitignore
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - one quick test here and I'm happy to merge
I have tested this item ✅ successfully on 9c389a2 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30056. |
Thanks! |
Closes #29968
Summary of Changes
READY FOR TESTING
A new custom fatal error page to replace the default symfony error page.
Joomla 4 now uses
symfony/error-handler
component for handling fatal errors WHEN DEBUG MODE IS OFF (or in this case, when its not yet been turned on or off byJConfig
)This is NOT a replacement error page or a duplication of
template/system/error.php
. That file will continue to be used for all the reasons its used already. It cannot be used when a fatal error occurs on boot up (and doesnt in Joomla 4 before this PR) because it relies on Joomla being booted, working, and relies on the whole stack working.fatal-error.html
is displayed when things are REALLY bad and unrecoverable only. Like your database server being offline.This page is only used during fatal unrecoverable errors, normally at boot time, when it cannot be assumed any of Joomla is working (certainly not a Factory or application running, certainly not a language object available)
Upstream Dependancy
It relies on upstream changes I made in the
symfony/error-handler
component that have been approved and merged, and that have been made specifically to support the ability of Joomla to render such a custom fatal page and Joomla will beone of the very firstthe first project to customise the Symfony default error page, when usingsymfony/error-handler
as a component in another non-symfony stack.Testing Instructions
Install Joomla 4 from 4.0-dev
merge/apply #31743 (running
npm i
ornode build/build.js --build-pages
to be sure)apply this PR - run
composer self-update --1 && composer update
(with v1 of composer)edit configuration.php and change class JConfig to be class JConfigBroken
Visit your home page.
Actual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request
Documentation Changes Required
New screenshots?
This page needs some content https://docs.joomla.org/J4.x:FatalError