IQBall
/
Application-Web
6
0
Fork 0
Code Issues 49 Pull Requests Packages Projects 5 Releases Wiki Activity
Labels Milestones

error-view #10

Merged
vivien.dufour merged 10 commits from error-view into master 1 year ago
Conversation 0 Commits 10 Files Changed 8
vivien.dufour commented 1 year ago
Owner

Add an error view for error 404, 401 and 403

Add an error view for error 404, 401 and 403
vivien.dufour added 8 commits 1 year ago
continuous-integration/drone/push Build is passing Details
b6d8ae551c conflict resolution
continuous-integration/drone/push Build is passing Details
9f902c431a add error 401 and 403 on error page
continuous-integration/drone/push Build is passing Details
b87aed643d center in the middle of the page the error text
maxime.batista approved these changes 1 year ago
public/index.php Outdated
$router->setBasePath($basePath);
$sampleFormController = new SampleFormController(new FormResultGateway($con), $twig);
$errorController = new \App\Controller\ErrorController(new FormResultGateway($con), $twig);
maxime.batista commented 1 year ago

No need to instanciate a this class here

No need to instanciate a this class here
vivien.dufour marked this conversation as resolved
src/Controller/ErrorController.php
class ErrorController
{
private FormResultGateway $gateway;
maxime.batista commented 1 year ago

why use a gateway in something that only displays an error view ?
Moreover, it is not used anywhere in the class, you should remove it

why use a gateway in something that only displays an error view ? Moreover, it is not used anywhere in the class, you should remove it
vivien.dufour marked this conversation as resolved
src/Controller/ErrorController.php
/**
* @param FormResultGateway $gateway
*/
public function __construct(FormResultGateway $gateway, Environment $twig)
maxime.batista commented 1 year ago

Remove the constructor, the $twig will be passed to the displayError function directly, and made static.

Remove the constructor, the `$twig` will be passed to the displayError function directly, and made static.
vivien.dufour marked this conversation as resolved
src/Controller/ErrorController.php Outdated
$this->twig = $twig;
}
public function displayError($error) {
maxime.batista commented 1 year ago

As this class is an utilitary class that proposes functions to display an error view, you should make this function static, and add a new Environment: $twig parameter;

As this class is an utilitary class that proposes functions to display an error view, you should make this function static, and add a new `Environment: $twig` parameter;
vivien.dufour marked this conversation as resolved
src/Controller/ErrorController.php Outdated
public function displayError($error) {
$error = array("error" => $error);
try {
echo $this->twig->render('error.html.twig', $error);
maxime.batista commented 1 year ago

You can use $this->twig->display instead of echoing the result of render :

- echo $this->twig->render('error.html.twig', $error);
+ $this->twig->display('error.html.twig', $error);
You can use $this->twig->display instead of echoing the result of render : ```diff - echo $this->twig->render('error.html.twig', $error); + $this->twig->display('error.html.twig', $error); ```
vivien.dufour marked this conversation as resolved
src/Views/error.html.twig Outdated
<meta charset="UTF-8">
<title>Error</title>
{% block stylesheets %}
maxime.batista commented 1 year ago

useless block use, you should remove it

useless block use, you should remove it
vivien.dufour marked this conversation as resolved
src/Views/error.html.twig
h1 {
color: #da6110;
text-align: center;
margin-bottom: 15px}
maxime.batista commented 1 year ago 
-  		margin-top: 15px}
+		margin-top: 15px
+	}
```diff - margin-top: 15px} + margin-top: 15px + } ```
vivien.dufour marked this conversation as resolved
src/Views/error.html.twig Outdated
h2 {
text-align: center;
margin-bottom: 15px;
margin-top: 15px}
maxime.batista commented 1 year ago 
-  		margin-top: 15px}
+		margin-top: 15px
+	}
```diff - margin-top: 15px} + margin-top: 15px + } ```
vivien.dufour marked this conversation as resolved
src/Views/error.html.twig
<h2>Erreur 401 : vous devez vous authentifier afin d'accèder à cette page</h2>
{% elseif error == "403" %}
<h2>Erreur 403 : vous n'avez pas les autorisations nécessaires pour accèder à cette page</h2>
{% endif %}
maxime.batista commented 1 year ago

This is not easily extensible, if we want to add more error codes and/or message, we will need to modify this file.
You can simply do something like that :

- {% if error == "404" %}
-     <h2>Erreur 404 : cette page n'existe pas</h2>
- {% elseif error == "401" %}
-     <h2>Erreur 401 : vous devez vous authentifier afin d'accèder à cette page</h2>
- {% elseif error == "403" %}
-     <h2>Erreur 403 : vous n'avez pas les autorisations nécessaires pour accèder à cette page</h2>
- {% endif %}
+ <h2>{{ error_type }} : {{ error_message }} </h2>

You'll then need to pass those two new variables to the view's argument :

$twig->display("error.html.twig", ["error_type" => $error_type, "error_message" => $error_message])
This is not easily extensible, if we want to add more error codes and/or message, we will need to modify this file. You can simply do something like that : ```diff - {% if error == "404" %} - <h2>Erreur 404 : cette page n'existe pas</h2> - {% elseif error == "401" %} - <h2>Erreur 401 : vous devez vous authentifier afin d'accèder à cette page</h2> - {% elseif error == "403" %} - <h2>Erreur 403 : vous n'avez pas les autorisations nécessaires pour accèder à cette page</h2> - {% endif %} + <h2>{{ error_type }} : {{ error_message }} </h2> ``` You'll then need to pass those two new variables to the view's argument : ```php $twig->display("error.html.twig", ["error_type" => $error_type, "error_message" => $error_message]) ```
vivien.dufour marked this conversation as resolved
maxime.batista requested review from maxime.batista 1 year ago
maxime.batista requested changes 1 year ago
maxime.batista left a comment

I meant to request changes, not to approve it*.

I meant to request changes, not to approve it*.
mael.daim approved these changes 1 year ago
vivien.dufour added 1 commit 1 year ago
vivien.dufour force-pushed error-view from 4e94522295 to 03945892df 1 year ago
maxime.batista approved these changes 1 year ago
maxime.batista left a comment

very good 👍

very good :+1:
vivien.dufour merged commit 4aa2bde233 into master 1 year ago
vivien.dufour deleted branch error-view 1 year ago
vivien.dufour referenced this issue from a commit 1 year ago
error-view (#10)

Reviewers

mael.daim approved these changes 1 year ago
maxime.batista approved these changes 1 year ago
continuous-integration/drone/push Build is passing
Details
The pull request has been merged as 4aa2bde233.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
mael.daim
maxime.batista
Labels
Apply labels
Clear labels
bug
Something is not working duplicate
This issue or pull request already exists enhancement
New feature help wanted
Need some help invalid
Something is wrong question
More information is needed wontfix
This won't be fixed
No Label bug duplicate enhancement help wanted invalid question wontfix
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: IQBall/Application-Web#10
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'error-view'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes