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

[4.3] Fix Tinymce and Codemirror version #38614

Merged
merged 3 commits into from Sep 4, 2022

Conversation

heelc29
Copy link
Contributor

@heelc29 heelc29 commented Aug 27, 2022

Summary of Changes

With PR #38422 the versions of codemirror and tinymce was downgraded:
image
image

This PR updated these extensions back to:

Testing Instructions

code review

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.3-dev labels Aug 27, 2022
@richard67
Copy link
Member

I've just checked and can confirm that this PR here is complete, i.e. the version numbers in files "plugins/editors/codemirror/codemirror.xml" and "plugins/editors/tinymce/tinymce.xml" were not downgraded by PR #38422 and so still are right.

@brianteeman Could you have a look, too? Maybe I'm missing something?

@brianteeman
Copy link
Contributor

is this even needed. It is for the 4.3 branch and when the next upmerge is done it will be resolved. And hopefully then it will have the latest updates merged.

Note: When you want to update a package you only do npm update package and not blindly npm update everything
Maintainers whenever there is a pr with a package-lock update you need to take extra care to make sure the submitter didnt make this mistake.

@richard67
Copy link
Member

It is for the 4.3 branch and when the next upmerge is done it will be resolved.

@brianteeman Not necessarily because the PR which downgraded the versions on 4.3-dev was committed after the ones which upgraded them on 4.2-dev, so there will not be any conflict on the upmerge, and git will keep it as it is. So I think this PR here is good to make sure it goes the right way. But you are right when there will be an update on these places in the package lock on the 4.2-dev branch later. Then this PR here will not be needed anymore because that later change would go up with the next upmerge after that.

@richard67
Copy link
Member

richard67 commented Aug 27, 2022

Note: When you want to update a package you only do npm update package and not blindly npm update everything Maintainers whenever there is a pr with a package-lock update you need to take extra care to make sure the submitter didnt make this mistake.

Fully correct. The mistake has happened when PR #38422 was merged, and that PR changed also other dependencies which should be checked. When doing like @brianteeman said, the package lock should show only expected changes for the updated package and its dependencies.

The same applies to composer dependencies.

@richard67
Copy link
Member

I have tested this item ✅ successfully on d9df238


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38614.

@heelc29
Copy link
Contributor Author

heelc29 commented Aug 27, 2022

and that PR changed also other dependencies which should be checked

hotkeys-js was updated to 3.9.4
image

For information: I updated the lockfile with npm install codemirror@5.65.6 and npm install tinymce@5.10.5 and reverted the changes of semantic versioning under dependencies

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on d9df238


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38614.

@richard67
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38614.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 28, 2022
@obuisard
Copy link
Contributor

Thanks @heelc29. Can you update your branch so I can merge your changes? Thank you!

@brianteeman
Copy link
Contributor

@obuisard you should have the rights to update the branch.

@obuisard
Copy link
Contributor

@obuisard you should have the rights to update the branch.

Hi Brian, it does not seem like I do, unless it is possible other than from the Github interface.

@brianteeman
Copy link
Contributor

@obuisard then you need to speak to the other maintainers as they all can. Its crazy if you dont and it will take forever to get things merged. (unless the user has marked this as cannot be updated - can you update other PR?)

@obuisard
Copy link
Contributor

obuisard commented Aug 31, 2022

Actually, you are right, I can update branches on other PRs, just not the ones from @heelc29. Thank you Brian.

@heelc29
Copy link
Contributor Author

heelc29 commented Sep 1, 2022

Thanks @heelc29. Can you update your branch so I can merge your changes? Thank you!

@obuisard Done 😀

@obuisard obuisard added this to the Joomla! 4.3.0 milestone Sep 4, 2022
@obuisard obuisard merged commit fb13e4b into joomla:4.3-dev Sep 4, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 4, 2022
@obuisard
Copy link
Contributor

obuisard commented Sep 4, 2022

Thank you @heelc29 for the PR :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.3-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants