Giter Club home page Giter Club logo

Comments (31)

silvioprog avatar silvioprog commented on July 19, 2024

It's an interesting explanation. You could send a patch implementing the fix? I think it would be easier to understand.

from brookframework.

leledumbo avatar leledumbo commented on July 19, 2024

I'm quite interested so let me take the honor to implement it (see latest push in working branch)

from brookframework.

uaply avatar uaply commented on July 19, 2024

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.

silvioprog avatar silvioprog commented on July 19, 2024

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.

uaply avatar uaply commented on July 19, 2024

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.

leledumbo avatar leledumbo commented on July 19, 2024

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.

uaply avatar uaply commented on July 19, 2024

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.

uaply avatar uaply commented on July 19, 2024

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.

silvioprog avatar silvioprog commented on July 19, 2024

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.

silvioprog avatar silvioprog commented on July 19, 2024

@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.

uaply avatar uaply commented on July 19, 2024

Just commited by changes to "working" branch.

from brookframework.

silvioprog avatar silvioprog commented on July 19, 2024

Very nice guys, I'll see it later. :)

from brookframework.

silvioprog avatar silvioprog commented on July 19, 2024

@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.

uaply avatar uaply commented on July 19, 2024

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.

leledumbo avatar leledumbo commented on July 19, 2024

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.

uaply avatar uaply commented on July 19, 2024

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.

silvioprog avatar silvioprog commented on July 19, 2024

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:

capturar

from brookframework.

silvioprog avatar silvioprog commented on July 19, 2024

@leledumbo, I had an idea. I'll implement it and show for yours. ;) Please await me...

from brookframework.

silvioprog avatar silvioprog commented on July 19, 2024

That's my changes:

1 - (76f6f14);
2 - (0fb1243);
3 - (2e079ef);

from brookframework.

uaply avatar uaply commented on July 19, 2024

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.

silvioprog avatar silvioprog commented on July 19, 2024

You are correct. Could fix it there friend?

from brookframework.

uaply avatar uaply commented on July 19, 2024

Fixed, and I hope everywhere.

from brookframework.

leledumbo avatar leledumbo commented on July 19, 2024

@silvioprog: that's good, just don't forget to document BROOK_DEBUG.

from brookframework.

silvioprog avatar silvioprog commented on July 19, 2024

@uaply, thank you again! :)
@leledumbo, can you write it? My englysh isn't very good hehe...

from brookframework.

leledumbo avatar leledumbo commented on July 19, 2024

Problem is: where should it be documented? There's no proper documentation yet, only API reference and demos/examples.

from brookframework.

uaply avatar uaply commented on July 19, 2024

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.

silvioprog avatar silvioprog commented on July 19, 2024

All changes applied to branch master.

from brookframework.

uaply avatar uaply commented on July 19, 2024

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.

silvioprog avatar silvioprog commented on July 19, 2024

We can close this issue friends?

from brookframework.

uaply avatar uaply commented on July 19, 2024

BROOK_DEBUG is still not documented, but this is completely different issue. So we can close this.

from brookframework.

silvioprog avatar silvioprog commented on July 19, 2024

Thank you very much! 👍

from brookframework.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo 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.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.