phpCssEmbed icon indicating copy to clipboard operation
phpCssEmbed copied to clipboard

Enable Remote HTTP assets

Open mhawker opened this issue 8 years ago • 3 comments

@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 avatar Jun 14 '16 13:06 mhawker

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?

mhawker avatar Jun 14 '16 13:06 mhawker

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.

krichprollsch avatar Jun 15 '16 08:06 krichprollsch

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

mhawker avatar Jun 15 '16 16:06 mhawker