Comments (31)
It's an interesting explanation. You could send a patch implementing the fix? I think it would be easier to understand.
from brookframework.
I'm quite interested so let me take the honor to implement it (see latest push in working branch)
from brookframework.
You are so fast, so I can't keep up with modifications :)
I would rather just add a few things
--- orig\brookutils.pas 2013-08-01 23:40:13.468750000 +0300
+++ brookutils.pas 2013-08-02 00:24:09.968750000 +0300
@@ -88,9 +88,13 @@
Charset: BROOK_HTTP_CHARSET_UTF_8;
ContentType: BROOK_HTTP_CONTENT_TYPE_TEXT_HTML;
Language: BROOK_DEFAULT_LANGUAGE;
- Page404: ES;
+ Page404: // probably should be defined in 'brookhttpconsts.pas'
+ '<html><head><title>Page not found</title></head><body>' +
+ '<h1>404 - Page not found</h1></body></html>';
Page404File: ES;
- Page500: ES;
+ Page500: // probably should be defined in 'brookhttpconsts.pas'
+ '<html><head><title>Internal server error</title></head><body>' +
+ '<h1>500 - Internal server error</h1></body></html>';
Page500File: ES;
DirectoryForUploads: ES;
DeleteUploadedFiles: False;
and
--- orig\brookfclhttpappbroker.pas 2013-08-01 23:39:47.218750000 +0300
+++ brookfclhttpappbroker.pas 2013-08-02 00:28:42.546875000 +0300
@@ -130,12 +130,16 @@
R.CodeText := BROOK_HTTP_REASON_PHRASE_NOT_FOUND;
R.ContentType := FormatContentType;
- if FileExists(BrookSettings.Page404File) then
+ if (BrookSettings.Page404File <> ES) and FileExists(BrookSettings.Page404File) then
R.Contents.LoadFromFile(BrookSettings.Page404File)
- else if BrookSettings.Page404 <> ES then
+ else
R.Content := BrookSettings.Page404;
- R.Content := Format(R.Content, [BrookSettings.RootUrl]);
+ // template language should be consistent with Page500
+ R.Content := StringsReplace(R.Content, ['@root'],
+ [BrookSettings.RootUrl], [rfIgnoreCase, rfReplaceAll]);
+ // and how I could find out path of requested file to insert into message?
+
R.SendContent;
VHandled := true;
end;
@@ -147,20 +151,27 @@
begin
if not R.HeadersSent then begin
R.Code := BROOK_HTTP_STATUS_CODE_INTERNAL_SERVER_ERROR;
- R.CodeText := 'Application error ' + E.ClassName;
+ R.CodeText := BROOK_HTTP_REASON_PHRASE_INTERNAL_SERVER_ERROR;
R.ContentType := FormatContentType;
- if FileExists(BrookSettings.Page500File) then begin
+ if (BrookSettings.Page500File <> ES) and FileExists(BrookSettings.Page500File) then begin
R.Contents.LoadFromFile(BrookSettings.Page500File);
- R.Content := StringsReplace(R.Content, ['@error', '@trace'],
- [E.Message, BrookDumpStack], [rfIgnoreCase, rfReplaceAll]);
- end else if BrookSettings.Page500 <> ES then begin
+ R.Content := StringsReplace(R.Content, ['@error'],
+ [E.Message], [rfIgnoreCase, rfReplaceAll]);
+ if Pos('@trace',LowerCase(R.Content))>0 then
+ R.Content := StringsReplace(R.Content, ['@trace'],
+ [BrookDumpStack], [rfIgnoreCase, rfReplaceAll]); // DumpStack is slow and not thread safe
+ end else begin
+ R.Content := BrookSettings.Page500;
+ StackDumpString := '';
if BrookSettings.ContentType = BROOK_HTTP_CONTENT_TYPE_APP_JSON then begin
ExceptionMessage := StringToJSONString(E.Message);
- StackDumpString := StringToJSONString(BrookDumpStack(LE));
+ if Pos('@trace',LowerCase(R.Content))>0 then
+ StackDumpString := StringToJSONString(BrookDumpStack(LF)); // newlines in output shouldn't be dependend on server platform
end else begin
ExceptionMessage := E.Message;
- StackDumpString := BrookDumpStack;
+ if Pos('@trace',LowerCase(R.Content))>0 then
+ StackDumpString := BrookDumpStack;
end;
R.Content := StringsReplace(BrookSettings.Page500, ['@error', '@trace'],
[ExceptionMessage, StackDumpString], [rfIgnoreCase, rfReplaceAll]);
@@ -192,24 +203,11 @@
R.SendContent;
Exit;
end;
- case E.ClassName of
- 'EBrookHTTP404': HandleHTTP404;
- 'EBrookHTTP500': HandleHTTP500;
- end;
- if VHandled then
- Exit;
- if (R.ContentType = BROOK_HTTP_CONTENT_TYPE_TEXT_HTML) or
- (BrookSettings.ContentType = BROOK_HTTP_CONTENT_TYPE_TEXT_HTML) then
- begin
- VStr := TStringList.Create;
- try
- ExceptionToHTML(VStr, E, Title, Email, Administrator);
- R.Content := VStr.Text;
- R.SendContent;
- finally
- VStr.Free;
- end;
- end;
+ if E is EBrookHTTP404 then begin
+ HandleHTTP404;
+ end else begin
+ HandleHTTP500;
+ end
end;
initialization
from brookframework.
Hello @uaply, can you send patch for all brokers? (CGI, FastCGI ...)
This change is to be applied after or before @leledumbo's changes?
ps. I think we could add you as a committer, so you could apply the changes directly in the working branch.
from brookframework.
It should be applied after @leledumbo's. I just hoped he will review my code and make the same changes in all needed files. So I didn't patched all brokers. Github lacks very important feature – attaching files to issues. It's very annoying indeed. It would be much simpler to commit changes directly, I suppose.
from brookframework.
BrookSettings.PageXXXFile <> ES
before FileExists
should not be necessary, FileExists
will return false in case of empty string anyway
else if BrookSettings.PageXXX <> ES
is also not needed, even it might be dangerous since R.Content could have partial/undefined value from elsewhere (just to be sure).
and why did you remove everything from case E.ClassName of
till end? It's the fallback mechanism for the exception in case everything else is not set.
from brookframework.
There is the difference between comparing strings locally and checking files with FileExists. Running FileExists('')
will make system call and depending on platform it will return sooner or later with False. But there could be some implications of doing system call with invalid data (empty string). Maybe this is ok, but maybe there would be some side-effect. For example sleeping hard-disk will be woken up to check if there is such file in current directory (despite filename is empty). You will never know what system will do.
So if invalid system call could be avoided, it should be avoided.
Second check BrookSettings.PageXXX <> ES of course should be removed. I suppose R.Content is not set beforehand, and even so, error handler should have priority over that.
E.ClassName stuff I removed because EBrookHTTP500 exception is never raised. Instead raised actual exception, e.g. EDivByZero. So there are only two possibilities: in one case EBrookHTTP404 is raised, and you should show Page404, or in other case some unknown exception is raised and you should show Page500. You can easily check it yourself.
from brookframework.
Oh, you are right. I missed the case when headers already send end R.HeadersSent = True.
Anyway fallback mechanism should not expose dump of callstack to the ordinary visitors if something went wrong.
I think HandleHTTP500 can be refactored to have two possibilities. If headers are already send, it can just show the same message without http headers. Maybe this also apply to Page404. We need to provoke somehow such a error to see what would be better.
from brookframework.
Hello friends. Please, send yours commits first in "working" branch. Several programers download Brook via "master" branch, so, if your code have a bug, it will afect all users. After all changes in "working" branch stay stable, we merge ir to "master" branch.
from brookframework.
@uaply, now you can commit directly in "working" and "master" branch. Please commit fisrt in "working". You can ask @leledumbo about yours changes. I'll see your changes soon as possible. Thank you buddies!
from brookframework.
Just commited by changes to "working" branch.
from brookframework.
Very nice guys, I'll see it later. :)
from brookframework.
@uaply, I saw your new constants:
BROOK_HTTP_RESPONSE_TEMPLATE_NOT_FOUND =
'<html><head><title>Page not found</title></head><body>' +
'<h1>404 - Page not found</h1></body></html>';
BROOK_HTTP_RESPONSE_TEMPLATE_INTERNAL_SERVER_ERROR =
'<html><head><title>Internal server error</title></head><body>' +
'<h1>500 - Internal server error</h1></body></html>';
So, if you change BROOK_HTTP_RESPONSE_TEMPLATE_INTERNAL_SERVER_ERROR
to:
BROOK_HTTP_RESPONSE_TEMPLATE_INTERNAL_SERVER_ERROR =
'<html><head><title>Internal server error</title></head><body>' +
'<h1>500 - Internal server error</h1>' +
'<p style="color: red;" >Error: @error</p></body></html>';
I can update the Brook expert to generate the following content in default Brokers unit:
unit Brokers;
{$mode objfpc}{$H+}
interface
uses
BrookFCLCGIBroker;
implementation
end.
Nice?
ps. I haven't had time to test yours implementeds. :(
from brookframework.
I am still not sure if it's good idea to display to users what Exception occurred. That wouldn't help them much, because they probably don't know what EAbstractError stands for. Neither it will help developer, because it is the user who will see that message, not developer.
Developer on his side most probably override this message to see stack trace:
BrookSettings.Page500 :=
'<html><head><title>Internal server error</title></head><body>' +
'<h1>500 - Internal server error</h1>' +
'<p style="color: red;">Error: @error</p>'+
'<pre style="color: gray;">@trace</pre></body></html>';
And developer will probably use some kind of log file for not to loose any exceptions he couldn't see because of JavaScript or anything else on top of plain HTTP.
What I am trying to say the user should not see any details on Error.
And the developer should see all details.
And by default the framework should be tuned for users, not for developers.
from brookframework.
And by default the framework should be tuned for users, not for developers.
I don't agree with this one. A framework should be tuned for developers by default, because developers are the ones who touch and play with the framework most of the time (and yes, we need stack trace for debugging). Users only taste the release version.
from brookframework.
Ok, lets the framework be for developers. At least now it is clearly stated and this resolves my doubts how would be better to do.
from brookframework.
I am still not sure if it's good idea to display to users what Exception occurred. That wouldn't help them much, because they probably don't know what EAbstractError stands for. Neither it will help developer, because it is the user who will see that message, not developer.
But I didn't posted it with @trace
, I posted only with @error
, please see my comment again (#48 (comment)).
I'll commit it, will be better to understand. But we can change it if necessary. (I love GIT :) )
After my commit, the new broker unit generated will be:
from brookframework.
@leledumbo, I had an idea. I'll implement it and show for yours. ;) Please await me...
from brookframework.
That's my changes:
1 - (76f6f14);
2 - (0fb1243);
3 - (2e079ef);
from brookframework.
There are number of demos left which have something like
BrookSettings.Page404 := PublicHTMLDir + '404.html';
BrookSettings.Page500 := PublicHTMLDir + '500.html';
They should be corrected as Page404 → Page404File
from brookframework.
You are correct. Could fix it there friend?
from brookframework.
Fixed, and I hope everywhere.
from brookframework.
@silvioprog: that's good, just don't forget to document BROOK_DEBUG.
from brookframework.
@uaply, thank you again! :)
@leledumbo, can you write it? My englysh isn't very good hehe...
from brookframework.
Problem is: where should it be documented? There's no proper documentation yet, only API reference and demos/examples.
from brookframework.
It seems the only way to pass information about not-found file to Page404 handler is to use E.Message.
Exception should be raised like
raise EBrookHTTP404.Create(ARequest.PathInfo);
and then error handler will know url of not-found page.
Commited relevant changes.
from brookframework.
All changes applied to branch master.
from brookframework.
Problem is: where should it be documented? There's no proper documentation yet, only API reference and demos/examples.
PasDoc documentation system allows to add two custom files: Introduction and Conclusion. It is not very flexible, but at least some notes could be placed there (Intro is displayed as default page).
from brookframework.
We can close this issue friends?
from brookframework.
BROOK_DEBUG is still not documented, but this is completely different issue. So we can close this.
from brookframework.
Thank you very much!
from brookframework.
Related Issues (20)
- Why plugins projects on github to brookframework is closing? HOT 4
- Telegram plugin for brookframework HOT 15
- i18n HOT 4
- Version 4 ? HOT 14
- libbrook.pas in Source HOT 3
- tardigrade demos throwing errors in libbrook HOT 4
- Server requirements? HOT 4
- dopf & LastInsertID HOT 4
- httprouter.lpr throwing error HOT 2
- How am I supposed to free worker thread in brook daemon? HOT 9
- Use legacy application with tardigrade HOT 19
- CPU Activity is high for tardigrade projects BROOK4 HOT 4
- More documentation HOT 1
- Can't get POST request data HOT 13
- Brookframework with sagui (legacy) HOT 11
- HTTP Client HOT 1
- Where did the captcha plugin for 4.0 go? HOT 1
- Some suggestion about JTemplate? HOT 3
- [IDEA] New repository HOT 1
- EBrookHTTPServer: Failed to send data in request for HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from brookframework.