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

Minor migrated Webservice improvements #10485

Merged
merged 11 commits into from Sep 20, 2018

Conversation

Projects
None yet
6 participants
@sarjon
Member

sarjon commented Sep 19, 2018

Questions Answers
Branch? develop
Description? This PR add some small improvements regarding migrated webservice code.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? New webservice page, that is accessible through admin-dev/index.php/configure/advanced/webservice/ should works the same.

This change is Reviewable

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 19, 2018

Contributor

ping @colinegin do we have a new block to help people with Webservices? it could be the right pull request to add introduce it.

Contributor

mickaelandrieu commented Sep 19, 2018

ping @colinegin do we have a new block to help people with Webservices? it could be the right pull request to add introduce it.

@matks matks added the migration label Sep 19, 2018

@colinegin

This comment has been minimized.

Show comment
Hide comment
@colinegin

colinegin Sep 20, 2018

Not yet @mickaelandrieu , you can merge it like that !

colinegin commented Sep 20, 2018

Not yet @mickaelandrieu , you can merge it like that !

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Sep 20, 2018

Member

@mickaelandrieu done.

do you think its worth having $request object just to access $_SERVER information, which is not acutally related to request at all? here's a line:

if (strpos($request->server->get('SERVER_SOFTWARE'), 'Apache') === false) {

Member

sarjon commented Sep 20, 2018

@mickaelandrieu done.

do you think its worth having $request object just to access $_SERVER information, which is not acutally related to request at all? here's a line:

if (strpos($request->server->get('SERVER_SOFTWARE'), 'Apache') === false) {

*
* @return array Errors if any
*/
public function checkForErrors();

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

the interface is wrong, should be checkForErrors(Request $request = null)

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

the interface is wrong, should be checkForErrors(Request $request = null)

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

do you think its worth having $request object just to access $_SERVER information, which is not acutally related to request at all? here's a line:

I think we may only inject $request->server->get('SERVER_SOFTWARE') here.

Contributor

mickaelandrieu commented Sep 20, 2018

do you think its worth having $request object just to access $_SERVER information, which is not acutally related to request at all? here's a line:

I think we may only inject $request->server->get('SERVER_SOFTWARE') here.

{
public function __construct(
TranslatorInterface $translator,
Configuration $configuration,

This comment has been minimized.

@sarjon

sarjon Sep 20, 2018

Member

can we use ConfigurationInterface? since this class is in Core namepsace?

@sarjon

sarjon Sep 20, 2018

Member

can we use ConfigurationInterface? since this class is in Core namepsace?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

no, I want to keep getBoolean() access

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

no, I want to keep getBoolean() access

public function __construct(
TranslatorInterface $translator,
Configuration $configuration,
HostingInformation $hostingInformation,

This comment has been minimized.

@sarjon

sarjon Sep 20, 2018

Member

can I add interface for this class?

@sarjon

sarjon Sep 20, 2018

Member

can I add interface for this class?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

not in the scope of this pull request ;)

@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

not in the scope of this pull request ;)

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Sep 20, 2018

@mbadrani mbadrani added QA ✔️ and removed waiting for QA labels Sep 20, 2018

@mickaelandrieu mickaelandrieu merged commit 2085d65 into PrestaShop:develop Sep 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

Thanks everyone!

Contributor

mickaelandrieu commented Sep 20, 2018

Thanks everyone!

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