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

Fix NL replace (do not replace \r\n with two spaces) #328

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Jan 13, 2021

previously \r\n was replaced by 2 spaces

@aik099
Copy link
Member

aik099 commented Jan 23, 2021

Problem uncovered by this PR: we're not testing the normalization logic of the getText method, because:

  • tests haven't failed after the change in this PR
  • each of drivers does its own normalization (Zombie replaces any trailing whitespace with a single space; BrowserKit replaces \n with a single space and then replaces all trailing spaces with a single space; Selenium2 replace any line break with a single space)

Proposed next steps:

  1. decide what we can test (not sure if \r could properly be tested in headless drivers)
  2. add a new test and restart all driver repo builds to see which drivers actually comply
  3. rebase this PR to see how the Selenium2 driver is doing

src/Selenium2Driver.php Outdated Show resolved Hide resolved
@aik099 aik099 requested a review from stof January 23, 2021 17:22
@mvorisek mvorisek changed the title Fix NL replace (do not replace \r\n with two spaces) Improve getText() whitespaces normalization + trim ends Jan 23, 2021
@mvorisek mvorisek changed the title Improve getText() whitespaces normalization + trim ends Fix NL replace (do not replace \r\n with two spaces) Oct 16, 2022
@mvorisek
Copy link
Contributor Author

I have removed 4391f59 commit to not trim and keep this PR to bare minimum of the described problem.

@aik099
Copy link
Member

aik099 commented Oct 20, 2022

@mvorisek , now I like the change.

@stof , what about testing considering that each driver is doing it differently?

@aik099 aik099 merged commit 28f43ec into minkphp:master Nov 1, 2024
2 of 10 checks passed
@aik099
Copy link
Member

aik099 commented Nov 1, 2024

Looks good. Merging. Thanks @mvorisek .

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.

2 participants