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.

Sign up for GitHub

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

Jump to bottom

Replace string before redirect #212

Merged
merged 1 commit into from
Feb 2, 2022
Merged

Replace string before redirect #212

merged 1 commit into from
Feb 2, 2022

Conversation

shtrom
Copy link
Contributor

@shtrom shtrom commented Aug 28, 2019

No description provided.

@shtrom
Copy link
Contributor Author

shtrom commented Aug 28, 2019

@j0k3r early WIP on #198, but keen to hear your thoughts.

j0k3r
j0k3r reviewed Aug 28, 2019
View reviewed changes
Copy link
Owner

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

Good start!

src/Extractor/ContentExtractor.php Outdated Show resolved Hide resolved
src/Extractor/ContentExtractor.php Outdated Show resolved Hide resolved
src/Extractor/ContentExtractor.php Outdated Show resolved Hide resolved
src/Extractor/ContentExtractor.php Outdated Show resolved Hide resolved
src/Extractor/ContentExtractor.php Outdated Show resolved Hide resolved
@shtrom shtrom force-pushed the replace_string-before-redirect branch from 1c1d178 to a2da53e Compare August 29, 2019 11:58
@shtrom
Copy link
Contributor Author

shtrom commented Aug 29, 2019

@j0k3r, ok, fixed, added tests, and squashed. I have a failing integration test, but it's also on master:

1) Tests\Graby\GrabyFunctionalTest::testMultipage
Failed asserting that 'Radeon HD 7750/7770 : DirectX 11.1 & PCI-Express 3.0 accessibles ? Participez aux discussions Recevez des notifications Profitez du mode sombre Vous avez déjà un compte ? Connectez-vous Utilisateur Mot de passe J'ai oublié mon mot de passe 0 0 Clubic …' contains "Avec un peu de retard sur le planning".

/home/shtrom/src/graby/tests/GrabyFunctionalTest.php:301

FAILURES!
Tests: 231, Assertions: 1033, Failures: 1, Skipped: 1.

I'm now giving that a spin in wallabag (;

@shtrom
Copy link
Contributor Author

shtrom commented Aug 29, 2019

I'm now giving that a spin in wallabag (;

Works like a charm 👌

I hope it gets into 2.4 😉

@j0k3r j0k3r force-pushed the replace_string-before-redirect branch from a2da53e to 5167925 Compare November 12, 2019 15:08
@shtrom
Copy link
Contributor Author

shtrom commented Jan 29, 2020

@j0k3r do you think there are more changes needed?

@shtrom
Copy link
Contributor Author

shtrom commented Dec 11, 2020

@j0k3r do you think there is anything blocking this getting merged?

@shtrom @j0k3r
ContentExtractor: Split processStringReplacements out of process
515b69c 
Also split out prepareSiteConfig, as a private dependency.

HttpClient: process string replacements before following redirects

We inject an optional ContentExtractor into the HttpClient. If present,
it will be use to process the fetched body before following redirects.

Signed-off-by: Olivier Mehani <shtrom@ssji.net>
@j0k3r j0k3r force-pushed the replace_string-before-redirect branch from 10daef6 to 515b69c Compare February 2, 2022 13:19
@j0k3r
Copy link
Owner

j0k3r commented Feb 2, 2022

I'm so sorry for the too long delay about your PR @shtrom ...
I've rebased it against the master, fixed conflicts and linter issues.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 95.077% when pulling 515b69c on shtrom:replace_string-before-redirect into abe816a on j0k3r:master.

@j0k3r j0k3r merged commit fda6724 into j0k3r:master Feb 2, 2022
@j0k3r j0k3r added this to the 3.0.0 milestone Feb 2, 2022
@j0k3r j0k3r linked an issue Feb 2, 2022 that may be closed by this pull request
Allow to ignore meta refresh header #198
Closed
@shtrom
Copy link
Contributor Author

shtrom commented Feb 2, 2022

Heh, no worries. Glad it's in!

@shtrom shtrom deleted the replace_string-before-redirect branch February 2, 2022 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j0k3r j0k3r j0k3r left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.

Allow to ignore meta refresh header
3 participants
@shtrom @j0k3r @coveralls