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 script for fixing var_dir issues for images #2638

Closed

Conversation

mateuszbieniek
Copy link
Contributor

This is port of legacy script that fixes images var_dir issues.

@mateuszbieniek
Copy link
Contributor Author

Open questions:

  1. Should this script have the option to move files to new var_dir?
  2. Should this script clear cache at the end, or just inform the user that cache has to be cleared?

'--iteration-count=' . $iterationCount,
];

$process = new Process(
Copy link
Contributor

@andrerom andrerom May 13, 2019

Choose a reason for hiding this comment

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

So side comment, but with this, re-index command and probably also others, maybe we can find a way to expose reusable code for parallel processes? So one place where we can make sure any global arguments like siteaccess, debug, env, memory, ... is passed to php & bin/console.

Same goes for installers and need to run sub commands, like in Commerce installer PR.

WDYT @adamwojs @alongosz ?

Copy link
Member

Choose a reason for hiding this comment

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

So side comment, but with this, re-index command and probably also others, maybe we can find a way to expose reusable code for parallel processes? So one place where we can make sure any global arguments like siteaccess, debug, env, memory, ... is passed to php & bin/console.

Same goes for installers and need to run sub commands, like in Commerce installer PR.

Yeah, this seems to be very repetitive.

Copy link
Member

@glye glye left a comment

Choose a reason for hiding this comment

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

No blockers from me.

  1. Move files? No. Minimise what it does. Admins may feel safer moving stuff themselves with regular file commands. Besides, if the script did the whole job, it would need a diffferent name :)
  2. Clear cache? No. Again, minimise what it does, and let admin decide how and when to clear. But be very clear in info to admin.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

The direction here with a Gateway is good, however you need to apply it for all queries called directly from the Command. Otherwise you might end up having multiple transactions.

$query = $this->connection->createQueryBuilder();
$query
->update('ezcontentobject_attribute', 'oa')
->set('oa.data_text', ':text')
Copy link
Member

Choose a reason for hiding this comment

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

This line tells me that you haven't tested this with Postgres ;) It won't work.

protected function initialize(InputInterface $input, OutputInterface $output)
{
parent::initialize($input, $output);
$this->imageGateway->setConnection($this->db);
Copy link
Member

Choose a reason for hiding this comment

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

Warning: this will stop working on upper branch. At this point I'd refactor Gateway itself, or decorate it just to be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Script has to be refactored slightly for 2.x and onward, so maybe w can leave it as it is and change it for "upper braches"?

@mateuszbieniek
Copy link
Contributor Author

closed in favor: #2995

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

Successfully merging this pull request may close these issues.

4 participants