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

fixed ad blocking issue #9885

Merged
merged 3 commits into from
Nov 9, 2018
Merged

fixed ad blocking issue #9885

merged 3 commits into from
Nov 9, 2018

Conversation

loevgaard
Copy link
Contributor

Q A
Branch? 1.2, 1.3, master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets fixes #9884
License MIT

@loevgaard loevgaard requested a review from a team as a code owner October 30, 2018 08:15
lchrusciel
lchrusciel approved these changes Oct 30, 2018
View reviewed changes
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

It would be awesome, to test it somehow, but I do not see any easy way for it :(

@Zales0123 Zales0123 added the Potential Bug Potential bugs or bugfixes, that needs to be reproduced. label Oct 30, 2018
Zales0123
Zales0123 approved these changes Oct 30, 2018
View reviewed changes
Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

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

With some refactoring, we could extract path generation to a separate service and then easily mock the response... but I don't know should it be part of this PR :)

@loevgaard
Copy link
Contributor Author

Well, basically the random_bytes function should be preseeded with a value, just like you can do with i.e. the Carbon library. But this means that you either need to create a lib to do that or find one that does.

Or you could do what @Zales0123 says ;)

Do you want to do any of this in this PR? If so I probably need some pointers as to where you want the PathGenerator class or if you have a lib in mind, which one.

@Zales0123
Copy link
Member

We've just had a little discussion about this with @lchrusciel in the office 😄 We both think that service extraction would be the best choice, but on the other hand, it would be a BC break and lead to many extra things to do (deprecations, fallback to old function etc.)... I don't know is the value of such a test worth so many actions 😄 cc @Sylius/core-team

@loevgaard
Copy link
Contributor Author

Well, personally I probably wouldn't, but on the other hand it lowers the quality of this exact class ;) I don't know if you any system in place where you put issues like this for future implementation? :)

@pamil
Copy link
Contributor

pamil commented Oct 30, 2018

The idea of extracting the path generation logic reminds me of a PR I made some time ago, #3703.

pamil
pamil reviewed Oct 30, 2018
View reviewed changes
src/Sylius/Component/Core/Uploader/ImageUploader.php Outdated
*/
private function isAdBlockingProne(string $path): bool
{
return stripos($path, 'ad') !== false;
Copy link
Contributor

Choose a reason for hiding this comment

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

It filters out 11-12% of generated paths. Do adblockers block the images if ad is the directory name or also if ad is in the middle of a name?

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for case-insensitive check here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I mentioned in the intro I have not any ad blocker installed and I couldn't find any valid source when trying to google it, so basically I just made the assumption that it would filter both paths and filenames

src/Sylius/Component/Core/Uploader/ImageUploader.php Outdated Show resolved Hide resolved
@pamil pamil merged commit ed3b141 into Sylius:1.2 Nov 9, 2018
@pamil
Copy link
Contributor

pamil commented Nov 9, 2018

Thanks, Joachim! 🥇

@loevgaard
Copy link
Contributor Author

My pleasure!

@loevgaard loevgaard deleted the fix-ad-blocker-issue-9884 branch November 12, 2018 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pamil pamil pamil left review comments

@Zales0123 Zales0123 Zales0123 approved these changes

@lchrusciel lchrusciel lchrusciel approved these changes

Assignees
No one assigned
Labels
Potential Bug Potential bugs or bugfixes, that needs to be reproduced.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@loevgaard @Zales0123 @pamil @lchrusciel