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.

Sign up for GitHub

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

Fix some deprecations #3732

Merged
merged 9 commits into from
Jan 7, 2016
Merged

Fix some deprecations #3732

merged 9 commits into from
Jan 7, 2016

Conversation

bendavies
Copy link
Contributor

As per the title!

@pjedrzejewski
Copy link
Member

👍

Thank you Ben!

stloyd
stloyd reviewed Dec 16, 2015
View reviewed changes
src/Sylius/Bundle/TaxonomyBundle/Form/Type/TaxonSelectionType.php
);
} else {
if (!isset($value['class'])) {
$value['class'] = 'Sylius\Bundle\TaxonomyBundle\Form\DataTransformer\TaxonSelectionToCollectionTransformer';
Copy link
Contributor

Choose a reason for hiding this comment

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

TaxonSelectionToCollectionTransformer::class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this is just re-indenting the code.
i thought to fix these class references in a separate PR, and fix all of them at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

@bendavies Good plan, will be easier to review and merge. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#3736

@bendavies
Copy link
Contributor Author

are the test failures because of these changes?

@bendavies bendavies mentioned this pull request Dec 17, 2015
Merged
@bendavies
Copy link
Contributor Author

@pjedrzejewski any idea?

pamil
pamil reviewed Dec 17, 2015
View reviewed changes
src/Sylius/Bundle/CoreBundle/Cart/ItemResolver.php
@@ -127,7 +127,7 @@ public function resolve(CartItemInterface $item, $data)

// We use forms to easily set the quantity and pick variant but you can do here whatever is required to create the item.
$form = $this->formFactory->create('sylius_cart_item', $item, array('product' => $product));
$form->submit($data);
$form->handleRequest($data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure $data is an instance of Request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, have a look at the first line of this method.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case we can rename it to $request, as many would expect some array with... data :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very true, but i'd rather keep this PRs changes specific to fixing deprecations.
that can be done in another PR

@pamil
Copy link
Contributor

pamil commented Dec 17, 2015

@bendavies Logging errors on Travis is temporarily not working (see #3727), have you tried to run these failing scenarios locally by bin/behat? You can run a single scenario by bin/behat features/feature_file.feature:line_number. With default Behat configuration you may find something interesting in etc/build/behat-*.log files.

@bendavies
Copy link
Contributor Author

@pamil thanks i'll take a look

@bendavies
Copy link
Contributor Author

this isn't working at the moment because PUT form submittions are expected to be POST by the form type

@bendavies
Copy link
Contributor Author

build is fixed.

@bendavies bendavies force-pushed the deprecations branch 2 times, most recently from a4347fc to 13f2d3e Compare December 23, 2015 15:22
@bendavies
Copy link
Contributor Author

Fixing more deprecations.

@bendavies bendavies force-pushed the deprecations branch 2 times, most recently from 3910cdd to 76e455d Compare December 23, 2015 16:32
@bendavies
Copy link
Contributor Author

That's quite a few fixed.
There are others but they look to be a bit more work.

@michalmarcinkowski
Copy link
Contributor

@bendavies good job! Could you rebase?

@bendavies
Copy link
Contributor Author

@michalmarcinkowski done

@bendavies
Copy link
Contributor Author

looks like there is a change in the handling of handleRequest and submit, hence the api validation test failures

@bendavies
Copy link
Contributor Author

I've reverted the form handling changes, as this is getting a heavy refactor in #3762 anyway.

@bendavies
Copy link
Contributor Author

@michalmarcinkowski build passes!

pjedrzejewski pushed a commit that referenced this pull request Jan 7, 2016
Merge pull request #3732 from bendavies/deprecations
e3b46f6 
Fix some deprecations
@pjedrzejewski pjedrzejewski merged commit e3b46f6 into Sylius:master Jan 7, 2016
@pjedrzejewski
Copy link
Member

Thank you very much Ben! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bendavies @pjedrzejewski @pamil @michalmarcinkowski @stloyd