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

Jump to bottom

[TranslatorBundle] Added missing validation on translation file #2712

Merged
merged 1 commit into from Aug 20, 2020
Merged

[TranslatorBundle] Added missing validation on translation file #2712

merged 1 commit into from Aug 20, 2020

Conversation

krewetka
Copy link
Contributor

@krewetka krewetka commented Aug 6, 2020

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets 2710

@krewetka krewetka changed the title Added validation on translation file [TranslatorBundle] Added missing validation on translation file Aug 6, 2020
ProfessorKuma
ProfessorKuma suggested changes Aug 6, 2020
View reviewed changes
Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @, your PR needs some changes

  • It seems that you should have submitted to the latest minor branch.
  • This PR seems to need a milestone of a patch release.

@ProfessorKuma ProfessorKuma added this to the 5.5.6 milestone Aug 6, 2020
@krewetka
Copy link
Contributor Author

krewetka commented Aug 6, 2020

Hi everyone,

I am not sure to which branch I should put this PR. Let me know then I can rebase it to correct one.

acrobat
acrobat requested changes Aug 7, 2020
View reviewed changes
Copy link
Member

@acrobat acrobat left a comment

Choose a reason for hiding this comment

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

Thanks @krewetka! Can you rebase the PR against the 5.5 branch?

src/Kunstmaan/TranslatorBundle/Form/TranslationsFileUploadType.php Outdated
@@ -19,6 +19,9 @@ public function buildForm(FormBuilderInterface $builder, array $options)
$builder->add('file', FileType::class, [
'required' => true,
'label' => 'kuma_translator.form.upload_file_choose',
'constraints' => array(
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the short array syntax here? And add a trailing comma to the new NotBlank() line. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no problem :) I see two types mixed in this file

acrobat
acrobat requested changes Aug 7, 2020
View reviewed changes
src/Kunstmaan/TranslatorBundle/Form/TranslationsFileUploadType.php Outdated
@@ -19,6 +19,9 @@ public function buildForm(FormBuilderInterface $builder, array $options)
$builder->add('file', FileType::class, [
'required' => true,
'label' => 'kuma_translator.form.upload_file_choose',
'constraints' => array(
new NotBlank()
Copy link
Member

Choose a reason for hiding this comment

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

And there is a use Symfony\Component\Validator\Constraints\NotBlank; use missing

@acrobat acrobat linked an issue Aug 7, 2020 that may be closed by this pull request
Error where proceeding with import translation when file is not selected #2710
Closed
@krewetka krewetka changed the base branch from 5.6 to 5.5 August 10, 2020 07:17
ProfessorKuma
ProfessorKuma approved these changes Aug 10, 2020
View reviewed changes
Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @, your PR passed all our requirements.

Thank you for contributing!

@krewetka
Copy link
Contributor Author

All should be fixed now. Only name in my branch in fork is 5.6 but it is rebased on 5.5

@krewetka
Copy link
Contributor Author

@acrobat are you able to figure what happened with travis build?

Can't open ./.build/validate-composer.sh does not look like related to my changes.

Also The 'const_separation' fixer cannot be disabled because it is not enabled by your preset. 🤔

@acrobat
Copy link
Member

acrobat commented Aug 10, 2020

@krewetka It seems that your 5.5 branch was not up to date with the latest changes as the const_seperation fixer error was fixed in 024385d

And the Can't open ./.build/validate-composer.sh error does also seem to be related with the 5.6 branch as this check was only added since v5.6

@krewetka
Copy link
Contributor Author

@acrobat I rebased it on newest version of 5.5 and all checks are passed now. Only thing probably left is your approval.

@acrobat acrobat merged commit c81da84 into Kunstmaan:5.5 Aug 20, 2020
@acrobat
Copy link
Member

acrobat commented Aug 20, 2020

Thanks @krewetka! And congrats on your first contribution! 🎉

@acrobat acrobat mentioned this pull request Aug 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acrobat acrobat acrobat approved these changes

@ProfessorKuma ProfessorKuma ProfessorKuma approved these changes

Assignees
No one assigned
Labels
Type: Bugfix
Projects
None yet
Milestone
5.5.6
Development

Successfully merging this pull request may close these issues.

Error where proceeding with import translation when file is not selected
3 participants
@krewetka @acrobat @ProfessorKuma