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

[Beta 8] Links missing attributes like target, rel #1650

Closed
jordanjay29 opened this issue Nov 21, 2018 · 3 comments
Closed

[Beta 8] Links missing attributes like target, rel #1650

jordanjay29 opened this issue Nov 21, 2018 · 3 comments
Labels

Comments

@jordanjay29
Copy link
Contributor

This appears to be a regression bug (#247 or surprise implementation of flarum/issue-archive#382 ;) ) from Beta 7.x.

Demonstrated at discuss.flarum.org (for example):

<a href="https://github.com/flarum/core/blob/c34fcecf0380d130d1aeb0a5dc6155f07f885d87/src/Formatter/Formatter.php#L140">https://github.com/flarum/core/blob/c34fcecf0380d130d1aeb0a5dc6155f07f885d87/src/Formatter/Formatter.php#L140</a>
@clarkwinkelmann
Copy link
Member

clarkwinkelmann commented Nov 21, 2018

Wait... this means it's reverting this commit I made a while ago 4c55d27 #1432 and therefore will prevent configuring those link attributes via extensions.

Not sure if my commit was the cause of the issue (looks like it was then ?), but I don't think that simply reverting is wise here as we loose the extensibility option 😬

@tobyzerner
Copy link
Contributor

tobyzerner commented Nov 21, 2018

Unfortunately switching the order was the cause of this regression. Fixing regressions comes at a higher priority than adding new extensibility features, thus the process is:

  1. Revert the commit that caused the regression (i.e. revert back to beta 7 behaviour)
  2. Release beta 8
  3. Consider options to allow for extensibility without causing regression in beta 9. @clarkwinkelmann if you would be so kind as to create an issue I'll add it to the b9 milestone!

@clarkwinkelmann
Copy link
Member

@tobscure of course you're right. I'll check out how to re-implement this correctly.

wzdiyb pushed a commit to wzdiyb/core that referenced this issue Feb 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants