Skip to content

TIP-747: upgrade the PIM from sf 2.8 to 3#6374

Merged
skeleton merged 32 commits intomasterfrom
symfony-upgrade
Aug 3, 2017
Merged

TIP-747: upgrade the PIM from sf 2.8 to 3#6374
skeleton merged 32 commits intomasterfrom
symfony-upgrade

Conversation

@anaelChardan
Copy link
Copy Markdown
Contributor

@anaelChardan anaelChardan commented Jul 2, 2017

Upgrade the PIM to Symfony 3.3.6 and related dependencies too.
Main changes:

  • Yaml component is more strict now:
    • if a parameter is not surrounded by quote, it send a deprecated alert
    • some caracteres have to be protected by quote, like !=, >, >=
  • Form component change the way they build options. Before, the id of an option was the key and the label was the value. Now it's the reverse \o/
  • SerializerAwareNormalizer is now deprecated. We have to use the SerializerAwareTrait

Result of bench for medium catalog:

master - symfony 2.8

import 50000 new products: 51:05,68 total
export 50000 products: 11:25,21 total

master - symfony 3.3

import 50000 new products: 52:47,27 total
export 50000 products: 11:44,14 total

Description (for Contributor and Core Developer)

Definition Of Done (for Core Developer only)

Q A
Added Specs Todo
Added Behats Todo
Added integration tests Todo
Changelog updated Todo
Review and 2 GTM Todo
Micro Demo to the PO (Story only) Todo
Migration script -
Tech Doc -

Todo: Pending / Work in progress
OK: Done / Validated
-: Not needed

Comment thread composer.json Outdated
"symfony/monolog-bundle": "2.12.1",
"symfony/swiftmailer-bundle": "2.4.2",
"symfony/symfony": "~2.8",
"symfony/symfony": "3.3.0",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

3.3.2

@anaelChardan anaelChardan force-pushed the symfony-upgrade branch 2 times, most recently from f7d21f9 to 6612a97 Compare July 3, 2017 09:16
Comment thread composer.json Outdated
"symfony/swiftmailer-bundle": "2.4.2",
"symfony/symfony": "~2.8",
"symfony/security-acl": "3.0.0",
"symfony/symfony": "~3.2.0",
Copy link
Copy Markdown
Contributor

@skeleton skeleton Jul 3, 2017

Choose a reason for hiding this comment

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

nope, don't do that on require packages, we already done that by the past, and it's a nightmare when there are new minors versions, it can cause regression and are very hard to detect.
So fix it to 3.2.2 please :)

@anaelChardan anaelChardan force-pushed the symfony-upgrade branch 6 times, most recently from 3518c81 to 9e211b1 Compare July 13, 2017 08:03
@anaelChardan anaelChardan force-pushed the symfony-upgrade branch 2 times, most recently from 8f8aea6 to 65cb3e6 Compare July 17, 2017 17:37
@skeleton skeleton force-pushed the symfony-upgrade branch 6 times, most recently from 1ac5fba to 1d15b53 Compare July 25, 2017 13:23
@skeleton skeleton force-pushed the symfony-upgrade branch 8 times, most recently from c6a94da to 174904c Compare August 1, 2017 19:30
@skeleton skeleton changed the title TIP-747: Update libraries TIP-747: upgrade the PIM from sf 2.8 to 3 Aug 1, 2017
Comment thread app/AppKernel.php Outdated
/**
* @return string
*/
public function getRootDir()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

string return

Comment thread app/AppKernel.php Outdated
/**
* @return string
*/
public function getCacheDir()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

string return

Comment thread app/AppKernel.php Outdated
/**
* @return string
*/
public function getLogDir()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

string return

Comment thread bin/console Outdated
use Symfony\Component\Console\Input\ArgvInput;
use Symfony\Component\Debug\Debug;

// if you don't want to setup permissions the proper way, just uncomment the following P HP line
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

spaces P HP

Comment thread bin/console
// if you don't want to setup permissions the proper way, just uncomment the following PHP line
use Symfony\Bundle\FrameworkBundle\Console\Application;
use Symfony\Component\Console\Input\ArgvInput;
use Symfony\Component\Debug\Debug;
Copy link
Copy Markdown
Contributor

@ahocquard ahocquard Aug 1, 2017

Choose a reason for hiding this comment

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

weird to you use autoloading file after those use statements. Actually I didn't know that it could work. In the example on internet, it's often after.

public static function getAllLabels()
{
return self::$statusLabels;
return array_flip(self::$statusLabels);
Copy link
Copy Markdown
Contributor

@ahocquard ahocquard Aug 1, 2017

Choose a reason for hiding this comment

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

Why ? In the PR, a lot of keys/value are inverted and I don't understand why.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Symfony changed the way the options are built, labels are keys now :(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do not understand why they did that but anyway...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

me too.. does not seem logical, and more important, you can loose data if label is the same :(

$choices = [];
foreach ($channels as $channel) {
$choices[$channel['code']] = null !== $channel['label'] ? $channel['label'] : '[' . $channel['code'] . ']';
$choices[null !== $channel['label'] ? $channel['label'] : '[' . $channel['code'] . ']'] = $channel['code'];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wooo ternary party 💃


/**
* @param Request $request
* @return Request
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

null|Request

_renderCriteria: function(el) {
const inKey = __('pim.grid.choice_filter.label_in_list');
const emptyKey = __('pim.grid.choice_filter.label_empty');
const notEmptyKey = __('pim.grid.choice_filter.label_not_empty');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just for my personal JS culture, why ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ask to @anaelChardan ^^

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Me too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Answered to @momoss in DM.

It was because I thought I should inverts also in JS and in JS we can't build dynamically a JSON key. But it does not seems useful now.

Comment thread web/app.php
require_once __DIR__.'/../app/AppKernel.php';
//require_once __DIR__.'/../app/AppCache.php';

require __DIR__.'/../vendor/autoload.php';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the web app in prod is the only file in Symfony 3.3 that should load bootstrap.php.cache.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this file has been copied directly from a sf 3.3 fresh installation. They load bootstratp.php.cache file only if the version of PHP is < 7. see https://github.com/symfony/symfony-demo/blob/master/web/app.php#L22

Copy link
Copy Markdown
Contributor

@ahocquard ahocquard Aug 2, 2017

Choose a reason for hiding this comment

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

Yes, but this is a demo not focus on performance.
https://symfony.com/doc/current/performance.html#use-bootstrap-files

If we don't provide it in pim-community-dev, no problem, but we should at least put it in the standard edition.

Copy link
Copy Markdown
Contributor

@ahocquard ahocquard Aug 2, 2017

Choose a reason for hiding this comment

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

Ok sry I'm wrong:

symfony/symfony-docs#7357
symfony/symfony-standard#1030

I've seen the opposite on some blogs.

@skeleton skeleton merged commit 3015ac1 into master Aug 3, 2017
@skeleton skeleton deleted the symfony-upgrade branch August 3, 2017 17:31
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.

5 participants