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

symfony/lock required in non-dev #104

Merged
merged 1 commit into from
Oct 20, 2019
Merged

Conversation

n3o77
Copy link
Contributor

@n3o77 n3o77 commented Oct 14, 2019

otherwise audit:clean and audit:schema:update can’t be executed

composer.json Outdated Show resolved Hide resolved
@DamienHarper
Copy link
Owner

Could you please update doc accordingly

Copy link
Contributor

@maxhelias maxhelias left a comment

Choose a reason for hiding this comment

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

I think the lock should remain in require-dev, but do a check before using the feature to be able to throw an exception to warn the user to add it on dependencies

@n3o77
Copy link
Contributor Author

n3o77 commented Oct 16, 2019

@maxhelias i would only agree on keeping it in require-dev if it wasn't required for the first installation steps. As it's now possible again to use default doctrine commands to update the database the docs could be changed to use those. Then we could mention the symfony/lock install when describing multi-db setups or cleaning up old logs.

@DamienHarper how would you like to handle this?

@n3o77 n3o77 force-pushed the lock-fix branch 2 times, most recently from 0b3c038 to dcd4439 Compare October 16, 2019 11:47
@DamienHarper
Copy link
Owner

@maxhelias this is already the case and documented in the cleaning section of the documentation (only) as of now.

In order to keep dependencies minimal and given the fact that (AFAIK):

  • most users don't clean their audit tables, they don't need the lock component.
  • schema update (in an upgrade scenario) is done in dev environment and using migrations most of the time.
  • the requirement is always tested at runtime and the exception thrown when the requirement is not satisfied is self explanatory.

So, we don't need to make this dependency a hard requirement => let's keep it in require-dev section of composer.json for now but we need to add a notice about the requirement in the audit table creation section of the documentation.

@DamienHarper DamienHarper changed the title [BUG] symfony/lock required in non-dev symfony/lock required in non-dev Oct 16, 2019
@n3o77 n3o77 force-pushed the lock-fix branch 2 times, most recently from 0a56048 to 9ede526 Compare October 16, 2019 12:29
@n3o77
Copy link
Contributor Author

n3o77 commented Oct 16, 2019

@DamienHarper i changed the file so it's supporting ^3.4 || ^4.0 in dev

DamienHarper
DamienHarper previously approved these changes Oct 16, 2019
@maxhelias
Copy link
Contributor

Prevent users with an exception at the beginning of the execution command like this :

if (!class_exists(Lock::class)) {
    throw new LogicException('The Lock component is required to use this command. Try running "composer require symfony/lock".');
}

Is a must have in addition to the documentation

@DamienHarper
Copy link
Owner

@maxhelias this is already enforced by using the LockableTrait

@maxhelias
Copy link
Contributor

Indeed, I had not seen 👍

@DamienHarper
Copy link
Owner

@n3o77 we only miss the notice in the doc to merge this PR

@n3o77
Copy link
Contributor Author

n3o77 commented Oct 16, 2019

@DamienHarper sorry, i thought you already changed that. I'll update in a few min

doc/12-upgrading.md Outdated Show resolved Hide resolved
@n3o77 n3o77 force-pushed the lock-fix branch 2 times, most recently from beeaa7c to fb43d48 Compare October 18, 2019 18:15
@n3o77
Copy link
Contributor Author

n3o77 commented Oct 18, 2019

I moved the symfony/lock module to require again and removed the notice that the package needs to be installed.

@DamienHarper DamienHarper merged commit ba53186 into DamienHarper:master Oct 20, 2019
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.

3 participants