contact form backend - #247
OStefan2001 wants to merge 6 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #247 +/- ##
=============================================
+ Coverage 60.58% 62.34% +1.75%
- Complexity 558 592 +34
=============================================
Files 91 95 +4
Lines 2187 2289 +102
=============================================
+ Hits 1325 1427 +102
Misses 862 862 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
again factory ? |
|
@alexmerlin please confrim that the code is correct here , i feel that i am missing something |
| ])); | ||
| } | ||
|
|
||
| return new RedirectResponse('/contact/?contact=sent#contact-form', 303); |
There was a problem hiding this comment.
You should not use hardcoded URLs.
Instead, generate the URL and pass it to RedirectResponse.
You can leave RedirectResponse with the default status 302 (Found) instead of using 303 (See other).
Or, if you need to set a different one, use status code constants from Fig\Http\Message\StatusCodeInterface.
| public const array TOPICS = [ | ||
| 'migration' => 'Migration', | ||
| 'project' => 'Project work', | ||
| 'oss' => 'Open source', | ||
| 'other' => 'Something else', | ||
| ]; |
There was a problem hiding this comment.
To me, this calls for an ENUM.
There was a problem hiding this comment.
Since this handler serves all static pages, you do the contact_success etc for all those pages.
Why not created a dedicated handler to use for displaying the form?
alexmerlin
left a comment
alexmerlin
left a comment
There was a problem hiding this comment.
I would swap the two handler names:
src/App/src/Handler/GetContactCreateFormHandler.php->src/App/src/Handler/GetCreateContactFormHandler.phpsrc/App/src/Handler/PostContactCreateHandler.php-> src/App/src/Handler/PostCreateContactHandler.php
So, it follows the general pattern we use across Dotkernel - see example.
| ['contact' => 'sent'], | ||
| 'contact-form' | ||
| ), | ||
| StatusCodeInterface::STATUS_SEE_OTHER |
There was a problem hiding this comment.
As I said:
You can leave RedirectResponse with the default status 302 (Found) instead of using 303 (See other).
Any reason for keeping 303?
| } catch (Throwable) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Shouldn't you log the error at this point?
| $app->get('/contact/', [GetContactCreateFormHandler::class], GetContactCreateFormHandler::TEMPLATE); | ||
| $app->post('/contact/', [PostContactCreateHandler::class], 'app::create-contact'); |
There was a problem hiding this comment.
Since the two contact pages now have their own handlers, you should name the routes here individually.
See naming convention example in Dotkernel Admin:
- for the route presenting the form, use
app::create-contact-form(example) - for the route processing the form, use
app::create-contact(example)
Also, this means removing contact from the static routes array in config/autoload/local.php.dist.
3 participants
Update $contactEmail from config/autoload/mail.global.php