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

Implement image source parser #10864

Merged
merged 3 commits into from Oct 8, 2018

Conversation

Projects
None yet
4 participants
@sarjon
Member

sarjon commented Oct 4, 2018

Questions Answers
Branch? develop
Description? Legacy ImageManager returns images as img tags. This parser helps to parse img path and makes it usable in migrated pages.
Type? new feature
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? Travis should be green, nothing manually to test.

This change is Reviewable

preg_match('@src="([^"]+)"@', $imageTag, $path);
if (empty($path)) {

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 4, 2018

Contributor

if (empty($path[1])) because it's the only index you really care =)

return null;
}
preg_match('@src="([^"]+)"@', $imageTag, $path);

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 4, 2018

Contributor

Just for me, why @ and not / like above?

This comment has been minimized.

@sarjon

sarjon Oct 4, 2018

Member

whats the different between them? im not really good at regex. :/

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 4, 2018

Contributor

There is no difference, it's just the delimiter. By experience the best one is ~ this is a character you rarely use :P
And using the same is just to be consistent, that's why I said "Just for me" ^^

This comment has been minimized.

@sarjon

sarjon Oct 5, 2018

Member

updated to be consistent. 👍

'/my-shop/path/to/my_image.jpg',
],
[
'<img src="../path/to/my_image.jpg">',

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 4, 2018

Contributor

what about testing

  • ../../../../../../../
  • ../.././
  • ./../../../
    Ok it's weird but can happened :P

This comment has been minimized.

@sarjon

sarjon Oct 4, 2018

Member

ohh you evil man, you made me update regex again... :p

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 4, 2018

Contributor

I can help you, I love regex.

This comment has been minimized.

@sarjon

sarjon Oct 4, 2018

Member

i've updated it, you may check it out. :)

sarjon added some commits Oct 4, 2018

@sarjon sarjon referenced this pull request Oct 5, 2018

Open

Migrate languages list #10839

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Oct 5, 2018

@matks I let you have the final check ;)

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 8, 2018

@matks can you review? :)

@matks

This comment has been minimized.

Contributor

matks commented Oct 8, 2018

@sarjon What can I say ? You got an Interface and a Unit Tests. If all PRs can be like this one, life would be a lot easier ^^

@matks matks merged commit 2a67c54 into PrestaShop:develop Oct 8, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@matks matks added this to the 1.7.6.0 milestone Oct 9, 2018

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