Skip to content
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

[WIP] ContactUs package #143

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

unckleg
Copy link
Contributor

@unckleg unckleg commented Aug 31, 2017

I have problem for sending POST onto Web Action.
I need to provide form filtering/error preview for ContactUsAction (Created view script, form, routes).
If someone can see where i came up and provide some hints.

TODO:

  • Write tests for whole package parts (Admin/Web).
  • Support for reCAPTCHA / MailChimp

@unckleg
Copy link
Contributor Author

unckleg commented Aug 31, 2017

Also is there any need for providing composer.json file as part of this PR ?

'contact_id' => (string) $this->contact_id,
'name' => (string) $this->name,
'email' => (string) $this->email,
'phone' => $this->phone,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can put (string) here too.

*
* @return array|\ArrayObject|null
*/
public function getById($contactId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here (and in all code generaly) you can use PHP7 return type.
there will be recommended to return ContactUs entity.
In that case you should check if result is null and throw exception if it is.
Mapper will return ContactUs by default.

We are planning to add return types in all code :)

@tasmaniski
Copy link
Member

Sry for the delay I hope that next time I will be faster.

What you can improve:

  • Use return types in all methods for cleaner and stable code. Specially when you need to return entity object

  • From the service, move Mapper's methods in Mapper, like:
    $this->mapper->insert($data); or $this->mapper->select([...]);
    this should goes to mapper.
    In same case use entity for passing the data. for eg.

service:
$this->mapper->insertContact($contactUsEntity); 

mapper:
$this->insert($contactUsEntity->getArray());

Everything else is perfect :)

@unckleg
Copy link
Contributor Author

unckleg commented Oct 9, 2017

Thanks for feedback, I'll send PR soon :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants