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

Fix issues inserting Widgets with nested WYSIWYGs #20174

Merged
merged 2 commits into from
Apr 23, 2019
Merged

Fix issues inserting Widgets with nested WYSIWYGs #20174

merged 2 commits into from
Apr 23, 2019

Conversation

molovo
Copy link
Contributor

@molovo molovo commented Jan 10, 2019

Description (*)

This PR fixes incorrect functionality when inserting widgets which contain a WYSIWYG field.

(Thanks to @EduardTd for pointing me in the right direction).

Fixed Issues (if relevant)

  1. Widgets with a WYSIWYG parameter fail when inserting them into a WYSIWYG in a form. #19742: Widgets with a WYSIWYG parameter fail when inserting them into a WYSIWYG in a form.
  2. custom widget with wysiwyg problem on insert widget via pages or blocks #13409: custom widget with wysiwyg problem on insert widget via pages or blocks

Manual testing scenarios (*)

Steps taken are the same used to reproduce #19742

(Code examples in this gist)

  1. Create a WYSIWYG block as shown in Wysiwyg.php
  2. Create a widget with a parameter which uses the WYSIWYG block as shown in widget.xml
  3. Edit a CMS page, and insert the new widget into the content WYSIWYG.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@molovo
Fix issues inserting Widgets with nested WYSIWYGs
3bcf0d8 
This PR is a potential fix for issues magento#19742 and magento#13409 (Thanks to @EduardTd for pointing me in the right direction).

I've tested this briefly in my own install and it appears to resolve the issue. Submitting this PR so that the issue can be tested fully.
@magento-engcom-team
Copy link
Contributor

Hi @molovo. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Jan 10, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@VladimirZaets VladimirZaets self-assigned this Jan 10, 2019
@molovo
Use wysiwygAdapter instead of global tinyMCE variable
d4e0c28 
I realised that tinyMCE is already being required as the wysiwygAdapter, so I've updated the code to point to that dependency instead. Still works.
@molovo molovo mentioned this pull request Jan 22, 2019
Closed
@magento-engcom-team magento-engcom-team added this to the Release: 2.3.1 milestone Jan 23, 2019
@magento-engcom-team
Copy link
Contributor

Hi @sidolov, thank you for the review.
ENGCOM-3953 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

@molovo thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@okorshenko okorshenko removed this from the Release: 2.3.1 milestone Jan 28, 2019
@VasylShvorak VasylShvorak self-assigned this Mar 1, 2019
@VasylShvorak
Copy link
Contributor

Hi @molovo there are some problems after testing PR:
Case 1:

  • Create a WYSIWYG block as shown in Wysiwyg.php
  • Create a widget with a parameter which uses the WYSIWYG block as shown in widget.xml
  • Go to Content->Pages
  • Edit Home Page
  • Click Content->Insert Widget icon
  • Select created widget
  • Add some text to wysiwyg field
  • Click Insert Widget
  • Check Content
    ✔️ Widget icon is shown
    20174
  • Go to Home Page
    ❌ Widget content isn't displayed

Case 2:

  • Create a WYSIWYG block as shown in Wysiwyg.php
  • Create a widget with a parameter which uses the WYSIWYG block as shown in widget.xml
  • Go to Content->Pages
  • Edit Home Page
  • Click Content->Insert Widget icon
  • Select created widget
  • Add image into wysiwyg field
  • Click Insert Widget
  • Check Content
    ❌ Broken widget icon is shown instead of inserted image
    20174admin

@sidolov
Copy link
Contributor

sidolov commented Mar 15, 2019

@molovo , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

@sidolov sidolov closed this Mar 15, 2019
@ghost
Copy link

ghost commented Mar 15, 2019

Hi @molovo, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@molovo
Copy link
Contributor Author

molovo commented Mar 18, 2019

@sidolov this PR only addresses the issues with selecting the correct WYSIWYG to insert the widget into.

Regarding the errors described by @VasylShvorak above, I'm unable to reproduce Case 1 - after saving the page and clearing the cache the widget appears correctly on the front end.

Case 2 occurs before applying this patch, and is caused by bugs elsewhere in the code messing up the encoding of the widget snippet which is inserted. IMO this PR should be merged, and that error addressed in a separate PR.

@molovo molovo reopened this Mar 18, 2019
@ghost ghost assigned orlangur Apr 3, 2019
@magento-engcom-team
Copy link
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-4664 has been created to process this Pull Request

@VasylShvorak VasylShvorak self-assigned this Apr 15, 2019
@VasylShvorak
Copy link
Contributor

✔️ QA passed

@nmalevanec nmalevanec self-assigned this Apr 16, 2019
@magento-engcom-team magento-engcom-team mentioned this pull request Apr 23, 2019
Closed
@magento-engcom-team magento-engcom-team merged commit d4e0c28 into magento:2.3-develop Apr 23, 2019
@m2-assistant
Copy link

m2-assistant bot commented Apr 23, 2019

Hi @molovo, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

taskula pushed a commit to Hypernova-Oy/magento2 that referenced this pull request Apr 23, 2019
@sivaschenko
ENGCOM-4664: Fix issues inserting Widgets with nested WYSIWYGs magent…
19c7bc9 
…o#20174
@magento-engcom-team magento-engcom-team added this to the Release: 2.3.2 milestone Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VladimirZaets VladimirZaets VladimirZaets requested changes

@orlangur orlangur orlangur approved these changes

@sidolov sidolov sidolov approved these changes

Assignees

@orlangur orlangur

@nmalevanec nmalevanec

@VasylShvorak VasylShvorak

Labels
Area: Lib/Frontend Progress: accept Release Line: 2.3
Projects
None yet
Milestone
Release: 2.3.2
Development

Successfully merging this pull request may close these issues.
9 participants
@molovo @magento-engcom-team @magento-cicd2 @VasylShvorak @sidolov @orlangur @VladimirZaets @okorshenko @nmalevanec