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 existence of parse_ini_file at installation time #15619

Closed
wants to merge 1 commit into from
Closed

[4.0] Check for existence of parse_ini_file at installation time #15619

wants to merge 1 commit into from

Conversation

PhilETaylor
Copy link
Contributor

@PhilETaylor PhilETaylor commented Apr 27, 2017

first part of fixing #15587 is to not allow new installations of Joomla that dont meet the minimum requirements (E.g. parse_ini_file is required for Joomla, therefore cannot be disabled

Pull Request for Issue # .

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

first part of fixing #15587 is to not allow new installations of Joomla that dont meet the minimum requirements (E.g. parse_ini_file is required for Joomla, therefore cannot be disabled
@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@Bakual
Copy link
Contributor

Bakual commented Apr 27, 2017

Imho we can't make it a requirement in J3, but we should in J4.

However we can show it as a recommend setting for J3

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 4d99cda


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15619.

@infograf768
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15619.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 27, 2017
@PhilETaylor

This comment was marked as abuse.

@infograf768 infograf768 removed the RTC This Pull Request is Ready To Commit label Apr 27, 2017
@infograf768
Copy link
Member

changed to pending


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15619.

@infograf768
Copy link
Member

I am the one who pointed to this issue and the solution in the maintainers chat.

I take off RTC indeed ( was too fast) as you used && instead of ||

The case we have is with people who do have parse_ini_string but not parse_ini_file, therefore please upgrade this PR to fit.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@infograf768
Copy link
Member

However Joomla 3.7.1 will NOT require parse_ini_file (if PR #15620 is accepted) and therefore this PR doesnt need to be made anyway and can be discarded.

Wrong imho, we still need to inform people at Installation time that something is wrong concerning their parse_ini whether it is file OR string.

@mbabker
Copy link
Contributor

mbabker commented Apr 27, 2017

If you change https://github.com/PhilETaylor/joomla-cms/blob/4d99cdaa107ac8242d87809646697322ea53b722/installation/model/setup.php#L291 to have a non-null value then when getPhpOptionsSufficient runs for "hard" requirements the INI parser availability won't cause it to fail over. Basically the same way we deal with it for ext/mcrypt.

@brianteeman
Copy link
Contributor

brianteeman commented Apr 27, 2017

Don't forget that a large percentage of new installation are not created with the installer

@mbabker
Copy link
Contributor

mbabker commented Apr 27, 2017

Ya well we can only cater for what we can control.

Even if we revert the "hard" requirement, having the check raise a notice if one of the two are missing would be good. That said, it should probably hard fail if both are missing unless we're willing to accept the dependency to the FOF code, which it seems some aren't willing to use a library we ship within core.

@PhilETaylor

This comment was marked as abuse.

@brianteeman
Copy link
Contributor

Crazy to ship AND Use a library but prevent it's use

@brianteeman
Copy link
Contributor

It is used

@PhilETaylor

This comment was marked as abuse.

@infograf768
Copy link
Member

I tested in real world and && works We get a "No" while || does not (after disabling parse_ini_file in php.ini)
Therefore this PR works here.

Any other debate concerns #15620
I change again my test to OK

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 4d99cda


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15619.

@Bakual
Copy link
Contributor

Bakual commented Apr 27, 2017

To my knowledge we required the parse_ini_string always in J3. So if that isn't allowed, Joomla wouldn't have worked properly already in J3.
The FOF class isn't really needed at all as we safely can use that native PHP method in J3.

@PhilETaylor

This comment was marked as abuse.

@Bakual
Copy link
Contributor

Bakual commented Apr 27, 2017

The reason was likely that you can use one function call instead of several and you don't need to do the QQ string replacing as parse_ini_file can handle that itself.
Plus on a sidenote it had better handling of "bad" files.
All in all a minor performance thing, nothing ground breaking.

@infograf768
Copy link
Member

No need to merge this as #15620 has been merged.

Let's consider it for 4.0 with some changes including new lang string AND default to hardcoded English phrase as otherwise user may only get the constant:
If we do, I suggest to separate the check for parse_ini_string from the one for parse_ini_file to make it clear at that time that it is the lack of this specific method which prevents installing.

Concerning update, we may also need to do something generally speaking, for example displaying after update a hardcoded error specifying the issue.

@mbabker
Copy link
Contributor

mbabker commented May 3, 2017

Even with #15620 merged this should still go into 3.x in some form. Right now our install checks are only looking for parse_ini_string() support. In theory if one put that method into the disabled functions list but left parse_ini_file() out of it (I could see it happening, people do dumb stuff), our install checks would fail even though they are probably in a usable configuration.

@infograf768
Copy link
Member

@mbabker
I have no problem implementing this in a different way. Separating both parse_ini methods and default to harcoded message in case of.

@brianteeman brianteeman modified the milestone: Joomla 4.0 Jun 8, 2017
@joomla-cms-bot joomla-cms-bot changed the title Check for existence of parse_ini_file at installation time [4.0] Check for existence of parse_ini_file at installation time Oct 26, 2017
@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.0 milestone Oct 26, 2017
@joomla-cms-bot joomla-cms-bot added this to the Joomla 4.0 milestone Jan 17, 2018
@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.0 milestone Jan 17, 2018
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.

None yet

6 participants