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

Fixing MangaHere #33

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

twluo
Copy link

@twluo twluo commented Sep 30, 2017

MangaHere made some pages to their websites which was causing the urls to be fetch incorrectly. In order for this configuration to work we need to edit the websites that it supports. I wasn't sure how to do that so if anyone can give me any help it would be appreciate it. For now I made a hotfix: https://github.com/twluo/AMR/blob/e314df3a29f2f23df35312107ff5976131854f49/js/mgEntry.js#L523-L525


This change is Reviewable

@braiam
Copy link
Member

braiam commented Oct 1, 2017

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


a discussion (no related file):
I haven't dig through the code on this one, but there are some notes that can make this fix more straightforward:

the chrome-extension that appears on the object returned by the mirror script is due mangahere using protocol relative url (fetching with curl the html can confirm this), to which the parser helpfully completes using the protocol reserved for extensions. So fixing this earlier should be possible on the response object.

A better fix would be to force all urls on AMR to not be protocol relative and append a function to all mirrors to default to https or http and make it a switchable option defaulting to http when nothing is present.

A long term fixing would be moving all the ajax calls to internal helper functions which accepts same input the ajax calls have and returns the object for further processing. Which would allow us to just store the path on the mangas object and localStorage, reducing it size and allowing us to be more flexible when sites moves domains or upgrade to https.

For now, I would accept this PR if the solution is introduced earlier on the function (on the objResponse) and instead of replacing chrome-extension, it simply tackles the protocol to the urls that doesn't have them.


Comments from Reviewable

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