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

Message::buildText(): Trim each text line. #84

Merged
merged 15 commits into from
Jun 2, 2021
Merged

Message::buildText(): Trim each text line. #84

merged 15 commits into from
Jun 2, 2021

Conversation

janbarasek
Copy link
Contributor

  • new feature
  • BC break? yes

In some cases, when converting HTML to text, blanks are left at the beginning and end of lines. This modification removes these unnecessary characters (saves storage when serializing messages and unifies formatting).

Screenshot from 2021-05-25 14-38-00

Screenshot from 2021-05-25 14-37-41

Thanks!

@@ -382,6 +382,7 @@ protected function buildText(string $html): string
]);
$text = Nette\Utils\Html::htmlToText($html);
$text = Strings::replace($text, '#[ \t]+#', ' ');
$text = implode("\n", array_map(static fn(string $line): string => trim($line), explode("\n", str_replace(["\r\n", "\r"], "\n", $text))));
Copy link
Member

@dg dg May 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static fn(string $line): string => trim($line) can be simply 'trim', or can't?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has one difference - PHP calls function internally, so weak typing is used instead of strict types
https://chat.stackoverflow.com/transcript/message/40320863#40320863

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, should I change it or will we let it go?

@dg dg merged commit 3bb2d2a into nette:master Jun 2, 2021
dg added a commit that referenced this pull request Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants