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.

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

Resolving early termination due to protocol URL in widget_script.phtml #132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

damjess
Copy link

@damjess damjess commented Feb 22, 2019

The protocol-relative URL was causing this script to terminate early on "compilation." The result in the static deployed files (causing an HTTP Error 500) appeared as:

<script type="text/javascript">
(function e(){var e=document.createElement("script");e.type="text/javascript",e.async=true,e.src="<?php echo getenv("TEST_ENV_WIDGET") ?: "</script>

This behavior was observed consistently across development, staging, and production environments.

As of December 2014, Paul Irish's blog on protocol-relative URLs says "2014.12.17: Now that SSL is encouraged for everyone and doesn’t have performance concerns, this technique is now an anti-pattern. If the asset you need is available on SSL, then always use the https:// asset."

My observation is that static resources served through Yotpo are already secure and thus should be enforced using https instead of protocol-relative urls. Changing the url specificity to https resolved the issue and is, in any regard, more secure.

The protocol-relative URL was causing this script to terminate early on "compilation." As of December 2014, [Paul Irish's blog](https://www.paulirish.com/2010/the-protocol-relative-url/) on protocol-relative URLs says "2014.12.17: Now that SSL is encouraged for everyone and doesn’t have performance concerns, this technique is now an anti-pattern. If the asset you need is available on SSL, then always use the https:// asset."

My observation is that static resources served through Yotpo are already secure and thus should be enforced using https instead of protocol-relative urls, anyways.
@damjess damjess changed the title Resolving early termination due to protocol URL Resolving early termination due to protocol URL in widget_script.phtml Feb 22, 2019
@damjess
Copy link
Author

damjess commented Apr 12, 2019

It's really sad how so many developers try to help Yotpo out by opening pull requests and you largely ignore them all while wreaking havoc on our sites. Your development practices are some of the most abhorrent I have seen to date. I will strongly advise my client to reconsider using your platform. I'm sick of the site-breaking bugs and error-log-filling warnings. Get your code together before subjecting paid users to it.

@wabakok-zz
Copy link

Hi,

Thanks for writing and I do apologize for the late response.
We're about to release a refactored version of our M2 version shortly which will also include the above.

pniel-cohen pushed a commit that referenced this pull request Dec 19, 2019
Fixed SyncStatus last_sync_date query
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.

2 participants