search

picocms / Pico

Pico is a stupidly simple, blazing fast, flat file CMS.
http://picocms.org/
MIT License
3.81k stars 615 forks source link

Enhancement: require mbstring polyfill #517

Closed JamesTheAwesomeDude closed 4 years ago

JamesTheAwesomeDude commented 4 years ago

c/f #509/551912485

On some webhosts, php-mbstring may not be available; including this 27KiB library remedies those cases.

PhrozenByte commented 4 years ago

I remember that I've had this discussion before, but for some reason I can't find it anymore :unamused: There are two issues with symfony/polyfill-mbstring in my opinion:

  1. symfony/polyfill-mbstring requires PHP's iconv extension. Even though iconv is a default extension, there definitely are installations out there which don't have iconv enabled. There's also a polyfill for iconv, however, symfony/polyfill-iconv is a pure PHP implementation, making it perform poorly; symfony/polyfill-mbstring is a partially native implementation at least. So, just to make this clear, using symfony/polyfill-mbstring is an improvement without any doubt. But it's no bullet-proof solution. Anyway, this isn't really a impediment.

  2. symfony/polyfill-mbstring isn't really a recommended solution, rather an "last resort" type of solution. Even though it's a partially native implementation, the downsides are still notable. It's always better to install the native PHP extension - and for most webservers you'll have to do this anyway, because requiring mbstring is very common. Thus I'm not sure whether there are actually (budget) hosters out there which don't have mbstring installed (the number of support tickets about this topic would be awful I guess). I'd rather assume that this is mostly an issue in self hosting environments (or in situations like "the IT department gave me access to this server", i.e. neither "real" hosting, nor self-hosting). The better solution always is to install the native extension. Requiring symfony/polyfill-mbstring by default just "hides" this issue. Requiring it on a case-by-case basis is still possible though.

This is no decision, I'm still struggling about it... Since we're currently in beta for Pico 2.1 it's definitely the right time to make this decision. More opinions would be appreciated!

JamesTheAwesomeDude commented 4 years ago

That definitely makes sense.

Seeing your concerns, I think I agree that (unconditionally) adding extra libraries that displace native, commonly-available functions with polyfills (however well-written), is probably not the right workaround.

I'm not sure how prevalent mbstring not being administratively available is; I know that https://webpages.uah.edu/ seems to lack it; but, other than that, I haven't really shopped around very much.

Perhaps it'd be a more elegant solution to just beef up the dependency documentation a bit? The biggest annoyance hurdle I had with this, was that nowhere in the User Documentation does it list any system requirements/dependencies beyond minimum PHP version.

If these were at least noted somewhere, I think it would enable the user to do appropriate troubleshooting without imposing the need for as much direct, technical debugging.

So, dependency-wise, what's missing from this list? If it's not much more than the following, would it be appropriate to just edit some extra deets into install.md?

PhrozenByte commented 4 years ago

Yeah, adding some note to the install instructions might be sufficient. Pico itself actually requires just PHP ≥ 5.3.6, however, Parsedown additionally requires the PHP extension mbstring and Parsedown Extra furthermore requires the PHP extension dom. That's it. mod_rewrite is, like URL rewriting in general, optional (unless you wanna use plugins implementing security stuff like authentication).