I'm unsure how this fell through the cracks, but WYSIWYG template should not be committed to our repo.

Tasks:

Remove the code
Remove the WYSIWYG template config file from /config/install
Fix our composer.json to pull WYSIWYG template library from the right place.
Open a PR to add the repo here too https://github.com/sparksi/sector-project

Comments

DanielVeza created an issue. See original summary.

xurizaemon’s picture

Status: Active » Needs review
StatusFileSize
new67.95 KB

Looks like the backstory to committing the templates plugin was recorded in #2945979: Allow use with composer without requiring ckeditor/templates.

Taking a stab at a patch for this, but feeling my way as I'm a little unclear about process when the libraries are referenced in composer.json for Sector, Sector Profile AND Sector Distribution :)

Related PRs:

* https://github.com/sparksi/sector-project/pull/5
* https://github.com/sparksi/sector-distribution/pull/10

danielveza’s picture

Fantastic! Thanks for jumping on this. This is a funny one since it does span Sector project and Sector distro.

The drupal.org and sector-distribution repos are essentially mirrors, so only things needed are the PRs on Sector project and this patch. :).

I'll review this today!

xurizaemon’s picture

Awesome! If you're able to document / share your process in reviewing this, that'll help me test & review changes to the "recipe" components in future.

dieuwe’s picture

Status: Needs review » Reviewed & tested by the community

This is good, although I got a few notices that some of the files in the templates directory had changed. Daniel just needs to check that the "libraries" directory is fully removed and nothing is left when he commits it.

This will require release notes and a notice that the repository will need to be added to all project based on Sector.

As for reviewing changes like this, composer changes are always a bit difficult. I tend to run a fresh checkout, apply the patch, run "composer install", and then make sure it succeeds with the right resulting structure. Then I manually copy the relevant result into an already built site to test the changes.

dieuwe’s picture

I have accepted xurizaemon's pull request to the sector-project default template on Github in preparation for Daniel to commit this patch here.

As it's going to require a change notice and release notes, this will probably be the final patch committed prior to the 5.0 stable release.

  • dieuwe committed 0d4575f on 5.0.x authored by xurizaemon
    Issue #3157340 by xurizaemon, dieuwe: Remove committed WYSIWYG template...
dieuwe’s picture

Status: Reviewed & tested by the community » Fixed

I re-rolled the patch due to composer.json changes and committed it.

Change notice to be published later today.

danielveza’s picture

Status: Fixed » Active

Uncaught Error: [CKEDITOR.resourceManager.load] Resource name "templates" was not found at "/profiles/contrib/sector-distribution/libraries/ckeditor/plugins/templates/plugin.js?t=qkfl1o".

This has broken node editing on HEAD. Lets fix that

danielveza’s picture

config/install/wysiwyg_template.settings.yml

Library path is set to the old path.

We need the following:

  • Remove that config setting. That should mean it falls back to a sane default
  • Hook update N to change the variable back to the right value on exisiting sites
danielveza’s picture

Status: Active » Needs review
StatusFileSize
new1.49 KB

This should do the trick

dieuwe’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies, update runs, and I can put a template in a node. Perfect.

I would recommend removing that empty line at the start of sector_install() prior to committing the patch, rather than just removing the whitespace.

  • dieuwe committed 6051e20 on 5.0.x authored by DanielVeza
    Issue #3157340 by DanielVeza, dieuwe: Remove committed WYSIWYG template...
dieuwe’s picture

Status: Reviewed & tested by the community » Fixed

Back to fixed we go (and hopefully stay there)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.