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

Fix/nex 336/schema change action abstraction #101

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

julien-sebire
Copy link
Member

This PR aims at proposing an abstraction for schema modification actions.
It has two levels of abstraction:

  • SchemaChangeAction : provides base template function: retrieving current schema, calling the changes, persist changes, error handling and returning report.
  • ResultStorageSchemaChangeAction : provides ResultStorage specific abstraction (persistence if provided by ResultStorage itslef), allowing an easy schema change.

try {
$this->writeSchema($oldSchema, $newSchema);
} catch (DBALException $e) {
Logger::w($e->getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think you can inject LoggerService and avoid usage of deprecated static methods?

try {
$table = $this->resultStorage->$creationMethodName($schema);
} catch (SchemaException $exception) {
// Table already exists. How can this happen, since we're doing this because

Choose a reason for hiding this comment

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

please remove comments and add logger->error('')

try {
$table = $schema->getTable($tableName);
} catch (SchemaException $exception) {
try {

Choose a reason for hiding this comment

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

please add log here as well

* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*
* Copyright (c) 2014-2017 (original work) Open Assessment Technologies SA (under the project TAO-PRODUCT);
*

Choose a reason for hiding this comment

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

for consistency, please remove the extra 2 lines at the end of the license

protected $persistence;

/**
* @param $params

Choose a reason for hiding this comment

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

i would specify the type

*
* @return Report
*/
public function __invoke($params)

Choose a reason for hiding this comment

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

not used, and no default value

*/
private function readSchema()
{
/** @var \common_persistence_sql_dbal_SchemaManager $schemaManager */

Choose a reason for hiding this comment

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

fqn simplified please

{
}

protected function getActionName()

Choose a reason for hiding this comment

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

do we really need this? it's a protected function that returns a constant, if yes maybe put it at the AbstractAction level

@julien-sebire julien-sebire changed the base branch from feature/NEX-168/uuid-primary-keys to develop October 14, 2019 16:56
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.

None yet

4 participants