Skip to content

Institution configuration options read-side #145

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

Merged
merged 11 commits into from
Jul 28, 2016

Conversation

arothuis
Copy link

This adds the read side of the institution configuration options: repository, entity and projector.

@arothuis arothuis changed the title Institution configuration options read Institution configuration options read-side Jul 27, 2016
public $institution;

/**
* @ORM\Column(type="boolean")
Copy link
Contributor

Choose a reason for hiding this comment

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

no doctrine types? Not saying you must introduce them, but if not it would deviate from the "normal" way afaik, thus that should be documented.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I want these to be Doctrine types. I want it to return VOs or it's going to be too inconsistent for comfort.

@DRvanR
Copy link
Contributor

DRvanR commented Jul 28, 2016

Might want to quickly introduce doctrine types for the VO's (also makes changes in the VO's format easier btw). But whatever you decide (docs or doctrine types), 👍 as it stands.


public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform)
{
return $platform->getVarcharTypeDeclarationSQL($fieldDeclaration);
Copy link
Author

Choose a reason for hiding this comment

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

This probably should not be a Varchar... ;)

@arothuis
Copy link
Author

arothuis commented Jul 28, 2016

@DRvanR; the options have been replaced with custom types and a migration has been added.

Sadly, the migration has accidentally been removed. I'm might rebase the specific commit just this once to keep a clean commit history.

@DRvanR
Copy link
Contributor

DRvanR commented Jul 28, 2016

sure thing, 👍

@arothuis arothuis force-pushed the feature/institution-configuration-options-read branch from 754c47c to cf1d35c Compare July 28, 2016 10:21
@arothuis arothuis merged commit 56e3926 into develop Jul 28, 2016
@arothuis arothuis deleted the feature/institution-configuration-options-read branch July 28, 2016 11:22
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