SabreDAV has hard requirement on mb_string #24906

Open
LukasReschke opened this Issue May 31, 2016 · 9 comments

Projects

None yet

5 participants

@LukasReschke
Member
LukasReschke commented May 31, 2016 edited

In https://github.com/owncloud/3rdparty/blob/c365711ac09143004d8aa991f3f7da58d90b54bb/sabre/vobject/lib/Property.php#L247-L248 SabreDAV has a hard dependency on mb_string.

While we have a fallback in place, that one is missing mb_strcut support as per https://github.com/symfony/polyfill/blob/854d515d887455562f860499a53dc75a26c79abe/src/Mbstring/Mbstring.php#L46-L57

This means that ownCloud simply fatal errors out with a blank error page. Can be easily tested by installing ownCloud newly on a Ubuntu 16.04.

@LukasReschke LukasReschke self-assigned this May 31, 2016
@LukasReschke LukasReschke added the bug label May 31, 2016
@LukasReschke
Member
LukasReschke commented May 31, 2016 edited

Strange. https://github.com/owncloud/core/blob/aba539703c0b0388f5ebf8757067bafab56774ce/lib/private/legacy/util.php#L741already has a check for this … let's see why this didn't trigger in…

I guess it's because of the fallback which in fact has mb_detect_encoding

@DeepDiver1975
Member

While we have a fallback in place, that one is missing mb_strcut support as per https://github.com/symfony/polyfill/blob/854d515d887455562f860499a53dc75a26c79abe/src/Mbstring/Mbstring.php#L46-L57

Do we know if there is any intention upstream to implement this function? THX

@PVince81 PVince81 closed this in #24907 May 31, 2016
@DeepDiver1975
Member

@LukasReschke any idea about upstream? Thx

@DeepDiver1975 DeepDiver1975 reopened this Jun 1, 2016
@guruz
Contributor
guruz commented Jun 15, 2016

FYI @evert

@evert
evert commented Jun 15, 2016

"Not being able to install mbstring" has never really been a real concern by any users I've heard of. mbstring is very common. Supporting a fallback does have drawbacks though. It basically creates a new dimension of potential bugs, and makes people installation significantly slower without warning them of this.

So to me the drawbacks definitely outweigh the benefits and I'm even tempted to say I rather not support installations at all that do this, including owncloud. Forcing mbstring to be installed guarantees an implementation that's tested and speedy.

@PVince81 PVince81 modified the milestone: 9.0.5, 9.0.4 Jul 18, 2016
@PVince81
Collaborator

So should we add an extra check for mb_string somewhere ?

Note that the blank page thing will be replaced with a proper exception string thanks to #26075

@PVince81 PVince81 modified the milestone: 9.0.6, 9.0.5 Sep 21, 2016
@PVince81 PVince81 modified the milestone: 9.0.7, 9.0.6 Oct 20, 2016
@PVince81
Collaborator

@DeepDiver1975 how critical is this ? Else => backlog

@PVince81 PVince81 modified the milestone: 9.0.8, 9.0.7 Nov 30, 2016
@PVince81
Collaborator
PVince81 commented Feb 3, 2017

apparently not critical, moving to backlog

@PVince81 PVince81 modified the milestone: backlog, 9.0.8 Feb 3, 2017
@PVince81
Collaborator
PVince81 commented Feb 3, 2017

the blank page should now show an exception on 10.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment