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

added je-marche form #308

Closed
wants to merge 5 commits into from
Closed

added je-marche form #308

wants to merge 5 commits into from

Conversation

antoineangot
Copy link

Bonjour,
J'ai fais une première version du formulaire "je-marche".

/**
* @var string
*
* @ORM\Column(name="actionType", type="string", length=50)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove all the columns names and all the column types if they are strings.

'L\'atelier du projet' => 'L\'atelier du projet',
'L\'action qui me ressemble' => 'L\'action qui me ressemble'
),
'attr' => array('class' => 'form--full')
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an issue in identation here and at some other places in this file.

$builder
->add('actionType', ChoiceType::class, array(
'choices' => array(
'La marche' => 'La marche',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should put canonical identifiers here instead of classical strings (ie la-marche instead of La marche to ensure a proper database storage).

/**
* JemarcheReport
*
* @ORM\Table(name="jemarche_report")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put the table name in plural form (jemarche_reports) to follow the convention?

use Doctrine\ORM\Mapping as ORM;

/**
* JemarcheReport
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this line.

/**
* @var string
*
* @ORM\Column(name="almostConvincedContact", type="text")
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to add asserts for form validation (I suppose you were planning it :) ).

public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefaults(array(
'data_class' => 'AppBundle\Entity\JemarcheReport'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the JemarcheReport::class notation.

{
return 'appbundle_jemarchereport';
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove these empty lines.

*/
public function getBlockPrefix()
{
return 'appbundle_jemarchereport';
Copy link
Contributor

Choose a reason for hiding this comment

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

app_jemarchereport (to follow convention)

*/
public function newAction(Request $request)
{
$jemarcheReport = new Jemarchereport();
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it JemarcheReport ?

/**
* @var string
*
* @ORM\Column(name="convincedContact", type="text")
Copy link
Contributor

Choose a reason for hiding this comment

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

SQL column names must be in snake_case, so the name attribute can be removed.

@antoineangot
Copy link
Author

fixed

@tgalopin tgalopin closed this Feb 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants