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

[4.0] check for libxml #31735

Closed
wants to merge 5 commits into from
Closed

[4.0] check for libxml #31735

wants to merge 5 commits into from

Conversation

brianteeman
Copy link
Contributor

PR for #29425

This is a real edge case discovered by @Fedik #29425 (comment)

libxml is enabled by default when compiling php and it cannot be disabled etc in a php.ini

The check I have put in place is the same one that myBB uses for the same issue.

The only questions are

  1. Did I put the check early enough
  2. Do we really need it for such an edge case

PR for joomla#29425

This is a real edge case discovered by @Fedik joomla#29425 (comment)

libxml is enabled by default when compiling php and it cannot be disabled etc in a php.ini

The check I have put in place is the same one that myBB uses for the same issue.

The only questions are
1. Did I put the check early enough
2. Do we really need it for such an edge case
@dgrammatiko
Copy link
Contributor

@brianteeman although this would work I think that the check needs to be in the Initial checks (compatibility layer):

public function getPhpOptions()

Also that would result to a better error view

@HLeithner
Copy link
Member

@brianteeman can you fix the cs plz

@Fedik
Copy link
Member

Fedik commented Dec 20, 2020

maybe better to place it in installation/includes/app.php or even installation/index.php

with some nice message like:

die(
str_replace(
array('{{PHP_VERSION}}', '{{BASEPATH}}'),
array(JOOMLA_MINIMUM_PHP, 'http://' . $_SERVER['SERVER_NAME'] . '/'),
file_get_contents(dirname(__FILE__) . '/../templates/system/incompatible.html')
)
);

I think that the check needs to be in the Initial checks (compatibility layer)

The problem the App does not even start if xml not available ;)

upd: the text in templates/system/incompatible.html can be updated to show more info, than just minimum php version

@dgrammatiko
Copy link
Contributor

maybe better to place it in installation/includes/app.php or even installation/index.php

Since all the checks from the Model could end up being foobar if any of these fail I guess beefing up these files is a more realistic approach. Yeah I know the code will end up being god awful but functional code is better than beautiful non functional code.

My 2c

@brianteeman brianteeman closed this Feb 3, 2021
@brianteeman brianteeman deleted the libxml branch February 3, 2021 19:32
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.

6 participants