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

Allow use of browserless server #2622

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

Conversation

mrleadhead
Copy link

Topic and Scope

I wanted to import recipes from gousto.co.uk however they require JavaScript to load the schema json. To achieve this I added a setting for the browserless address and adjusted the HTML download so that if the setting is set it will proxy the html download via browserless. https://github.com/browserless/browserless#hosting-providers

Open to making this more general allowing other services to be used. Browserless seemed the simplest tool currently.

Concerns/issues

There is an issue where the browserless address field in the settings page does not populate, I've reached the end of my vue and php skills I'm sure it's something simple. Feel free to make as many changes as you like my php skill is not great.

Formal requirements

There are some formal requirements that should be satisfied. Please mark those by checking the corresponding box.

  • [ X] I did check that the app can still be opened and does not throw any browser logs
  • [ X] I created tests for newly added PHP code (check this if no PHP changes were made)
  • [ X] I updated the OpenAPI specs and added an entry to the API changelog (check if API was not modified)
  • [ -] I notified the matrix channel if I introduced an API change

Comment on lines +16 to +19
// Check and fix `id` under `author`
if (array_key_exists('author', $json) && is_array($json['author']) && array_key_exists('id', $json['author'])) {
$json['id'] = strval($json['author']['id']);
}
Copy link
Author

Choose a reason for hiding this comment

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

Gousto put the ID under author to be difficult

@SnowyLeopard
Copy link

Would love to get this merged, what's needed to get this moving?

@christianlupus
Copy link
Collaborator

I quickly skimmed over the PR as I closed all open PRs for the maintenance release 0.11.3. I saw, that I would like to look closer on this.

Just one point I directly saw was the fact that the browserless approach was not using curl but file_get_contents. This is generally fine but caused some trouble in the past (we used file_get_contents there for all downloads) with broken encodings (especially Asian and kyrillic languages).

Also, I am not sure about a good architecture here. I am thinking about using polymorphism here to reuse the curl setup at best. But this just a quick glance at the code.

I would like to give it a good glance and see to a good integration of it all, if you do not mind. I find this interesting as it might solve some issues we have right now. However I was not aware of this and needed to get known to browserless.

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