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

Migrate System information page to Symfony #8254

Merged
merged 9 commits into from Sep 21, 2017

Conversation

@mickaelandrieu
Contributor

mickaelandrieu commented Aug 17, 2017

Questions Answers
Branch? develop
Description? Migration of the "Information System" page from Legacy to Symfony
Type? improvement
Category? BO
BC breaks? no
Deprecations? no (except for whom who override the old controller)
Fixed ticket http://forge.prestashop.com/browse/BOGOSS-42
How to test? Access Configure > Advanced Parameters > System Information

Capture (13/09/2017)
information_system

Important guidelines


This change is Reviewable

@eternoendless eternoendless added the WIP label Aug 17, 2017

@PrestaShop PrestaShop deleted a comment from prestonBot Aug 18, 2017

@aleeks

This comment has been minimized.

Show comment
Hide comment
@aleeks

aleeks Aug 18, 2017

Contributor

Don't forget to delete the old smarty template 👍

Contributor

aleeks commented Aug 18, 2017

Don't forget to delete the old smarty template 👍

@mickaelandrieu mickaelandrieu changed the title from [WIP] Migrate System information page to Symfony to Migrate System information page to Symfony Aug 18, 2017

@mickaelandrieu mickaelandrieu removed the WIP label Aug 18, 2017

@mickaelandrieu mickaelandrieu requested a review from eternoendless Aug 18, 2017

return Tools::apacheModExists('mod_instaweb');
}
/**

This comment has been minimized.

@eternoendless

eternoendless Aug 25, 2017

Member

What's "host mode"? This would be a good place to explain

@eternoendless

eternoendless Aug 25, 2017

Member

What's "host mode"? This would be a good place to explain

This comment has been minimized.

@eternoendless

eternoendless Aug 28, 2017

Member

After discussing the subject with @rGaillard, this is an old feature that we are going to drop. So we won't need to port this method after all 🙂

@eternoendless

eternoendless Aug 28, 2017

Member

After discussing the subject with @rGaillard, this is an old feature that we are going to drop. So we won't need to port this method after all 🙂

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2017

Contributor

We have decided to migrate legacy as it: this is really important. I think the removal of deprecated features is important but this can't be done during a critical task like this one.

@mickaelandrieu

mickaelandrieu Aug 28, 2017

Contributor

We have decided to migrate legacy as it: this is really important. I think the removal of deprecated features is important but this can't be done during a critical task like this one.

This comment has been minimized.

@eternoendless

eternoendless Aug 28, 2017

Member

Ok, we'll do it afterwards

@eternoendless

eternoendless Aug 28, 2017

Member

Ok, we'll do it afterwards

return function_exists('php_uname') ? php_uname('s').' '.php_uname('v').' '.php_uname('m') : '';
}
/**

This comment has been minimized.

@eternoendless

eternoendless Aug 25, 2017

Member

What's instaweb? This would be a good place to explain

@eternoendless

eternoendless Aug 25, 2017

Member

What's instaweb? This would be a good place to explain

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2017

Contributor

I'll glad to explain it, if I knew :) /c maybe @rGaillard knows?

@mickaelandrieu

mickaelandrieu Aug 28, 2017

Contributor

I'll glad to explain it, if I knew :) /c maybe @rGaillard knows?

This comment has been minimized.

@rGaillard

rGaillard Aug 28, 2017

Member

@mickaelandrieu some hosting providers disable some functions like exec() or this one.

@rGaillard

rGaillard Aug 28, 2017

Member

@mickaelandrieu some hosting providers disable some functions like exec() or this one.

*/
public function getUname()
{
return function_exists('php_uname') ? php_uname('s').' '.php_uname('v').' '.php_uname('m') : '';

This comment has been minimized.

@eternoendless

eternoendless Aug 25, 2017

Member

Why wouldn't php_uname exist? It's available since PHP 4 😛

@eternoendless

eternoendless Aug 25, 2017

Member

Why wouldn't php_uname exist? It's available since PHP 4 😛

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2017

Contributor

Was wondering the same, and why this check was done on PrestaShop on the first place: it appears that PHP is not always compiled with PHP, so php_uname can't exists.

@mickaelandrieu

mickaelandrieu Aug 28, 2017

Contributor

Was wondering the same, and why this check was done on PrestaShop on the first place: it appears that PHP is not always compiled with PHP, so php_uname can't exists.

This comment has been minimized.

@eternoendless

eternoendless Aug 28, 2017

Member

PHP is not always compiled with PHP

🙃

@eternoendless

eternoendless Aug 28, 2017

Member

PHP is not always compiled with PHP

🙃

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2017

Contributor

:D php_uname function is not always compiled with PHP

@mickaelandrieu

mickaelandrieu Aug 28, 2017

Contributor

:D php_uname function is not always compiled with PHP

This comment has been minimized.

@eternoendless

eternoendless Aug 28, 2017

Member

I couldn't find that anywhere, where did you see it? 🤔

@eternoendless

eternoendless Aug 28, 2017

Member

I couldn't find that anywhere, where did you see it? 🤔

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 28, 2017

Contributor

I don't remember, searching on google I think the main issue is that some hosters disable php_uname for security reasons: don't know why this can be a security issue.

Anyways, I don't want to risk a BC on it.

@mickaelandrieu

mickaelandrieu Aug 28, 2017

Contributor

I don't remember, searching on google I think the main issue is that some hosters disable php_uname for security reasons: don't know why this can be a security issue.

Anyways, I don't want to risk a BC on it.

This comment has been minimized.

@eternoendless

eternoendless Aug 28, 2017

Member

Huh, I only just discovered that there's a php.ini directive that can be used to disable any PHP function (and there's one for classes, too).

So I guess we can leave it as is.

@eternoendless

eternoendless Aug 28, 2017

Member

Huh, I only just discovered that there's a php.ini directive that can be used to disable any PHP function (and there's one for classes, too).

So I guess we can leave it as is.

/**
* @return string
*/
public function getUname()

This comment has been minimized.

@eternoendless

eternoendless Aug 25, 2017

Member

This is too generic. How about something like getOsInformation?

@eternoendless

eternoendless Aug 25, 2017

Member

This is too generic. How about something like getOsInformation?

Show outdated Hide outdated src/Adapter/Hosting/HostingInformation.php
Show outdated Hide outdated ...le/Resources/views/Admin/AdvancedParameters/system_information.html.twig
Show outdated Hide outdated ...le/Resources/views/Admin/AdvancedParameters/system_information.html.twig
Show outdated Hide outdated ...le/Resources/views/Admin/AdvancedParameters/system_information.html.twig
Show outdated Hide outdated ...ndle/Controller/Admin/AdvancedParameters/SystemInformationController.php
Show outdated Hide outdated ...ndle/Controller/Admin/AdvancedParameters/SystemInformationController.php

Comments fixed, thanks! Don't forget to add the "waiting for QA feedback" label :)

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 13, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 13, 2017

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 15, 2017

Contributor

@fatmaBouchekoua thanks for review, comments adressed :)

Contributor

mickaelandrieu commented Sep 15, 2017

@fatmaBouchekoua thanks for review, comments adressed :)

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 15, 2017

/**
* @return array
*/
public function getServerInformation()

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 requested changes Sep 20, 2017 edited

The page works, but the function CheckMissingOrUpdatedFiles::getListOfUpdatedFiles() needs to be modified. I don't know how it is possible, but there are some errors not caught by the Symfony error handler.

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Sep 21, 2017

Member

Changes requested made in order to go faster.

Member

Quetzacoalt91 commented Sep 21, 2017

Changes requested made in order to go faster.

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 21, 2017

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Sep 21, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 1
           

Complexity increasing per file
==============================
- classes/Link.php  2
         

See the complete overview on Codacy

codacy-bot commented Sep 21, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 1
           

Complexity increasing per file
==============================
- classes/Link.php  2
         

See the complete overview on Codacy

@Quetzacoalt91 Quetzacoalt91 merged commit 708ea09 into develop Sep 21, 2017

2 of 3 checks passed

codacy/pr Not so good... This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Quetzacoalt91 Quetzacoalt91 deleted the migration-system-information-page branch Sep 21, 2017

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91
Member

Quetzacoalt91 commented Sep 21, 2017

Thanks @mickaelandrieu 👌

@xBorderie xBorderie added this to the 1.7.3.0 milestone Sep 21, 2017

@xBorderie

This comment has been minimized.

Show comment
Hide comment
@xBorderie

xBorderie Sep 21, 2017

Contributor

Wow!

Contributor

xBorderie commented Sep 21, 2017

Wow!

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