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

Rating titles with whitespace results in broken ID attributes #5451

Closed
chicgeek opened this issue Jul 4, 2016 · 3 comments
Closed

Rating titles with whitespace results in broken ID attributes #5451

chicgeek opened this issue Jul 4, 2016 · 3 comments
Assignees
Labels
bug report Fixed in 2.2.x The issue has been fixed in 2.2 release line Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release

Comments

@chicgeek
Copy link
Contributor

chicgeek commented Jul 4, 2016

The review form uses the rating labels to output element attributes in the FE form.phtml template. These attribute values include IDs that should not include whitespace - this whitespace should be stripped out (likely with str_replace) for all (but one) instances of:

<?php echo $block->escapeHtml($_rating->getRatingCode()) ?>

The only instance that should not have whitespace is the FE-visible label, the second instance on L30:

<label class="label" id="<?php echo $block->escapeHtml($_rating->getRatingCode()) ?>_rating_label"><span><?php echo $block->escapeHtml($_rating->getRatingCode()) ?></span></label>

Steps to reproduce

This issue is present in 2.0.6, 2.0.7, and 2.1 (and likely earlier versions).

  1. In the admin panel, go to Stores > Attribute > Rating.
  2. Click Add new rating.
  3. Create new rating with a default value of test with space. Make it visible on all stores, enable it. Save.
  4. Clear caches and view the markup on the frontend.

Desired result

L30, with the first rating label whitespace replaced with non-whitespace characters:

<label class="label" id="test_with_space_rating_label"><span>test with space</span></label>

Actual result

L30, as-is:

<label class="label" id="test with space_rating_label"><span>test with space</span></label>

This produces an element with multiple IDs. While this is valid markup, the likelihood of unintentionally having multiple elements with the same ID (invalid markup) is fairly high.

Note

Earlier versions may require the patch that is mentioned in this issue to make ratings visible to stores: #4455

@andimov andimov self-assigned this Aug 8, 2016
@andimov andimov removed their assignment Sep 2, 2016
@AVoskoboinikov AVoskoboinikov self-assigned this Sep 13, 2016
@AVoskoboinikov
Copy link
Contributor

Hi @chicgeek

Thanks for contributing. I've created internal ticket (#MAGETWO-58339) to fix this. This issue will be closed after fix.

Have a good day!

@AVoskoboinikov AVoskoboinikov added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Sep 13, 2016
@AVoskoboinikov AVoskoboinikov removed their assignment Sep 14, 2016
@magento-engcom-team magento-engcom-team added 2.0.x Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development bug report Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed labels Sep 11, 2017
@magento-engcom-team magento-engcom-team added 2.2.x Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Oct 18, 2017
@magento-engcom-team
Copy link
Contributor

@chicgeek, thank you for your report.
We've created internal ticket(s) MAGETWO-58339 to track progress on the issue.

@magento-engcom-team
Copy link
Contributor

Hi @chicgeek. Thank you for your report.
The issue has been fixed in magento-engcom/magento2ce#1119 by @nmalevanec in 2.2-develop branch
Related commit(s):

The fix will be available with the upcoming 2.2.4 release.

@magento-engcom-team magento-engcom-team added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Feb 22, 2018
magento-cicd2 pushed a commit that referenced this issue Apr 28, 2020
Fixed Issues:
- MC-31679: FPC doesn't work with Varnish enabled
- MC-32145: Wishlist upgrade db update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report Fixed in 2.2.x The issue has been fixed in 2.2 release line Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

No branches or pull requests

7 participants