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

Enable Remote HTTP assets #17

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

Conversation

mhawker
Copy link

@mhawker mhawker commented Jun 14, 2016

@krichprollsch

OK, third times a charm (sorry).

This pull request adds the ability to work with remote HTTP assets or remote stylesheets. It does not modify the default functionality of the class in any way; enabling the embed of remote assets requires that the enableHttp method be called.

There are flags for just about every use-case that I can imagine. I documentation is complete. I'm pretty sure that everything is covered with unit tests except when the user specifically requests to embed remote font files (not default behaviour).

Thanks,

@mhawker
Copy link
Author

mhawker commented Jun 14, 2016

On the failed unit test: it works, but sometimes httpbin times out before sending the response.

Do you have any thoughts on how to test this more reliably?

@krichprollsch
Copy link
Owner

Hello @mhawker thank you for your great work.
I have to take time to look your PR in detail, just 3 remarks:

  • not sure with enabling http using the bit flags. Indeed, all the flags are useless if http is disabled. WDYT of using 2 params, one to enable http with a boolean, the other to have flags options, which could be not specific to http, but for all the process ?
  • the methods the set one flag and remove it are not a real need imo. I think you generally create one instance of cssenbed for your need, and you untouch it in the time. less code is better to maintain
  • please could you look at the scrutinizer report, it returns some interresting issues you could maybe address.

@mhawker
Copy link
Author

mhawker commented Jun 15, 2016

Good evening @krichprollsch

So, I tried to integrate your suggestions. The class is growing in size a lot; it's now close to 500 LoC. Do you think it needs a refactor or are you OK with the size?

For the API, I removed the set/unset options. The enableHttp method now accepts two arguments: a boolean to enable HTTP assets and a $flags argument for HTTP-specific options.

On the other hand, I added a setOptions method for options that could apply either to local assets or HTTP assets, which are embedding fonts, embedding svg files and url on error.

I had to add better mime type detection in order to get the options working for local assets. Please tell me what you think about the solution. I thought it was important to keep the class as a stand-alone rather than distributing a mime.types file as part of it. The method enableEnhandedMimeTypes will allow the user to specify a mime.types file and optionally download the one that Apache uses.

The scrutinizer score is still pretty low because some of the methods are too complex. I'll have to think about how to fix it. Do you have any suggestions?

Thanks again for your time and have a good one.

Matt

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