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

Updates s9e/text-formatter to 2.x #1982

Merged

Conversation

@tankerkiller125
Copy link
Member

tankerkiller125 commented Feb 1, 2020

Fixes #1979

Changes proposed in this pull request:

Updates s9e/text-formatter to the latest version, no changes other than composer.json were needed, tested all functionality still works and extensions such as fof/formatting still work as intended.

Reviewers should focus on:

  • Should this be in the beta.12 release or a following update
  • Any breaking changes I might have missed (tested emoji, fof/formatting, mentions, markdown and bbcode all working)
  • Please note that this change adds new markdown support (notably spoilers) when markdown extension enabled.

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
No additional changes required, tested with fof/formatting extension.
@luceos
luceos approved these changes Feb 3, 2020
@tankerkiller125 tankerkiller125 requested a review from franzliedke Feb 12, 2020
Copy link
Member

franzliedke left a comment

Thank you, sir!

@franzliedke franzliedke merged commit 17257aa into flarum:master Feb 14, 2020
10 checks passed
10 checks passed
PHP 7.1 / MySQL
Details
PHP 7.1 / MariaDB
Details
PHP 7.2 / MySQL
Details
PHP 7.2 / MariaDB
Details
PHP 7.3 / MySQL
Details
PHP 7.3 / MySQL (prefix)
Details
PHP 7.3 / MariaDB
Details
PHP 7.3 / MariaDB (prefix)
Details
WIP Ready for review
Details
continuous-integration/styleci/pr The analysis has passed
Details
wzdiyb added a commit to wzdiyb/core that referenced this pull request Feb 16, 2020
No additional changes required, tested with fof/formatting extension.
@luceos

This comment has been minimized.

Copy link
Member

luceos commented Feb 24, 2020

While deploying to discuss we encountered an issue in firefox. Possibly xslt is not supported in this browser, did that version update to 2 perhaps? We might need a polyfill for that.

@tankerkiller125

This comment has been minimized.

Copy link
Member Author

tankerkiller125 commented Feb 24, 2020

The following link is a side by side comparison of the render.js of s9e/text-formatter, on the left is the old one and on the right is the new one. https://gist.github.com/tankerkiller125/041361820156f76e0426bdf1e000429c/revisions I'm combing through but I've been unable to find where/why it might be erroring.

@askvortsov1

This comment has been minimized.

Copy link
Contributor

askvortsov1 commented Feb 24, 2020

Indeed, XSLT 2.0 isn't supported natively by firefox: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XSLT_2.0

Maybe we could add in the Saxon JS runtime as a workaround? http://www.saxonica.com/saxon-js/index.xml

@luceos

This comment has been minimized.

Copy link
Member

luceos commented Feb 24, 2020

Can either of you do a test with saxon as a solution? We can check whether that's wanted or not in the PR.

@askvortsov1

This comment has been minimized.

Copy link
Contributor

askvortsov1 commented Feb 24, 2020

I can do so later today and report back. Regardless, might be good to file an issue with s2e: if this breaks Firefox for us, it'll break it for pretty much everyone. I'll also look into xstl compat with other browsers

@askvortsov1

This comment has been minimized.

Copy link
Contributor

askvortsov1 commented Feb 24, 2020

After some experimentation, I'm not entirely sure that xlst2 is the issue (it doesn't look like s9e/text-formatter changed to it?) I'm running a local installation of beta 12 with only bundled extensions installed, and everything works fine for me on Firefox. @luceos what version of firefox were you able to reproduce this with? I'll try running with that, as well as installing all the Discuss extensions and seeing whether that changes anything.

@tankerkiller125

This comment has been minimized.

Copy link
Member Author

tankerkiller125 commented Feb 24, 2020

@askvortsov1 I also see the issue on Firefox 73.0.1 you can see it on https://nightly.flarum.site possibly.

@luceos

This comment has been minimized.

Copy link
Member

luceos commented Feb 24, 2020

72.0.2 (64-bit), just upgraded to 73.0.1 and having the same issue on nightly

@tankerkiller125

This comment has been minimized.

Copy link
Member Author

tankerkiller125 commented Feb 24, 2020

It looks like it may be related to sentry?
image
But I'm not sure, this may just be because it's erroring before sentry initializes.

@askvortsov1

This comment has been minimized.

Copy link
Contributor

askvortsov1 commented Feb 24, 2020

I'm running the same firefox, and it's working fine on the bare-bones local install. Can we try disabling sentry/other extensions on nightly?

On a brief tangent, I know that if I restart my docker container running flarum and fof/sentry can't connect to sentry, the whole site crashes. While that particular issue might be specific to my use of Docker, I wouldn't be surprised if some issue in sentry could be causing this.

Daniel, were you able to confirm on a local bare-bones installation, or just on discuss and nightly?

@luceos

This comment has been minimized.

Copy link
Member

luceos commented Feb 24, 2020

We have protection in place to prevent the removal of sentry on the demo sites. 🤔

@askvortsov1

This comment has been minimized.

Copy link
Contributor

askvortsov1 commented Feb 24, 2020

the odd thing is, the first error log in Matt's screenshot (the one's that just a colon) originates in s9e's library. Possibly the error is handled somewhere else in s9e, but sentry tries to report it and fails, and that blocks rendering of the buttons?

Unfortunately, ":" is not a very informative error message...

@tankerkiller125

This comment has been minimized.

Copy link
Member Author

tankerkiller125 commented Feb 24, 2020

Just tested without sentry, on Firefox 73.0.1, complete error, no tags list, we may need to revert until this issue gets fixed by the text-formatter lib.

@luceos

This comment has been minimized.

Copy link
Member

luceos commented Feb 24, 2020

@JoshyPHP is this something you encountered before? https://nightly.flarum.site with firefox. Seems somewhere this browser is not supported.

@JoshyPHP

This comment has been minimized.

Copy link
Contributor

JoshyPHP commented Feb 24, 2020

No, I've never heard of an issue related to a single colon. I read the comments but I haven't seen anything about the actual error. Is there an error message somewhere?

The templates use the same XSLT 1.0 as before.

@tankerkiller125

This comment has been minimized.

Copy link
Member Author

tankerkiller125 commented Feb 24, 2020

@JoshyPHP We're just seeing the colon error, nothing more. Upon loading in Chrome I'm able to access the s9e.TextFormatter global functions but on Firefox I can not. Even with webpack dev mode I'm unable to find the exact location of the error with the JS maps.

image

Were there any major changes to the JS I missed when upgrading from what would have been 1.X to 2.X?

@JoshyPHP

This comment has been minimized.

Copy link
Contributor

JoshyPHP commented Feb 24, 2020

I looked into it and the error message is caused by Firefox's XPath lexer which has difficulties parsing some expressions.

As a temporary workaround, you can manually alter the template used for [list] with this:

$configurator->tags['list']->template = str_replace('or', '+', $configurator->tags['list']->template);
@askvortsov1

This comment has been minimized.

Copy link
Contributor

askvortsov1 commented Feb 24, 2020

This seems to be a pervasive issue throughout the site (for example, this also breaks the reply/like links at the bottom of each post). I'm not sure that it would be feasible to manually patch around every instance, especially when some of them might be in 3rd party extensions

Ignore that, I misunderstood the proposed solution. Thanks a lot for looking into this!

@tankerkiller125

This comment has been minimized.

Copy link
Member Author

tankerkiller125 commented Feb 24, 2020

@askvortsov1 patching the BBCode extension resolved all issues in my dev environment.

@luceos I've created a patch to the BBCode extension flarum/bbcode#9

@JoshyPHP

This comment has been minimized.

Copy link
Contributor

JoshyPHP commented Feb 24, 2020

@tankerkiller125 Don't patch Flarum's extension, I'll fix the issue in my library directly and release 2.3.5 2.3.6 later today.

@tankerkiller125

This comment has been minimized.

Copy link
Member Author

tankerkiller125 commented Feb 24, 2020

@JoshyPHP even better :)

@tankerkiller125

This comment has been minimized.

Copy link
Member Author

tankerkiller125 commented Feb 24, 2020

I've just tested with the latest version of text-formatter and everything is working as expected now on Firefox, thanks @JoshyPHP

@luceos

This comment has been minimized.

Copy link
Member

luceos commented Feb 24, 2020

@JoshyPHP thanks for hopping into this so fast 🤗

@tankerkiller125 feel free to PR the minimum constraint for 2.3.6 if you want.

Great job!

imzhi pushed a commit to jh-technology-center/core that referenced this pull request Mar 9, 2020
No additional changes required, tested with fof/formatting extension.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.