Implementation of hook_requirements() breaks installation profiles by checking for a function (format_number_get_options()) provided by another module (format_number). I haven't tested, but that might actually break enabling formatted_number and format_number at the same time, too.

CommentFileSizeAuthor
#1 857928-1_requirements.patch2.05 KBalex_b
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alex_b’s picture

Status: Active » Needs review
FileSize
2.05 KB

I suggest to remove the hook_requirements() implementation and replace it with an entry in the README file.

markus_petrux’s picture

Status: Needs review » Closed (duplicate)

Sorry, it's been already discussed: #618238: hook_requirements() not working as expected during drupal installation using custom profile

To quote myself from the other issue: "Why drupal_load() returns FALSE when invoked during the installation of a custom profile? That looks like a bug in Drupal (or the custom profile) to me."

alex_b’s picture

Status: Closed (duplicate) » Needs review

I urge you to reconsider this patch. Sorry for being a pain here, but I am on a spot.

1)

Why drupal_load() returns FALSE when invoked during the installation of a custom profile? That looks like a bug in Drupal (or the custom profile) to me.

This is not a bug.

Requirements are checked at a time where much of the Drupal API simply cannot be assumed to be present. See this debug backtrace to get a sense of how early hook_requirements() is run on installation:

http://skitch.com/alexbarth/dp52c/requirements-problem-drupal

2)

Keeping the hook_requirements() implementation as-is means that formatted number cannot be used in installation profiles without patching Drupal core, without a serious refactoring of Drupal's installation process. Code that does not even have tests.

3)

The above is to be considered in the light of the fact that the requirements hook merely checks a specific version of a dependency. That's by far not common practice in Drupal.

Without this patch I won't be able to release the upcoming version of http://drupal.org/project/feeds_test . Formatted Number CCK is part of that test suite because I recently accepted a patch for Formatted Number integration by infojunkie #838018: Mapper for Formatted Number CCK. That patch includes tests that depend on FNCCK.

markus_petrux’s picture

Category: bug » feature
Status: Needs review » Closed (won't fix)

Sure, I can understand the situation. However, if I do not check for required modules correctly, then I may generate issues for those who forgot to install the proper version before this module, and Drupal does not provide another method to check for requirements.

This is a bug in Drupal core, and should be fixed there. Otherwise, we need to know what to do. I refuse to remove the requirements check here without an alternative that is clearly supported by those in charge on Drupal core as an official best practice to deal with these situations. Sadly, I do not have the time to start that war, otherwise I would have done the first time I found this issue.

Since we started a new discussion, I'm not going to say "dup" again, but I'm going to temporarily say the same I said in the other issue: as a feature request, this is won't fix. Drupal core needs to be fixed, or an official alternative needs to be documented on how to check requirements of this kind. Otherwise, we'll have issues from people not installing the correct requirements.

Note that not checking for correct requirements properly may lead to any kind of malfunction, which may even lead to loss of data. So, please sorry, but no, thanks.

infojunkie’s picture

Category: feature » bug
Status: Closed (won't fix) » Needs review

+1

I think the ability to include Formatted Number CCK in install profiles outweighs the strictness of dependency checking. The patch in the other issue #618238: hook_requirements() not working as expected during drupal installation using custom profile shifted the verification to runtime which seems reasonable. It's not just about feeds_test, but about any automated deployment scenario that uses FNCCK - which is a big deal for larger sites.

markus_petrux’s picture

Category: bug » feature
Status: Needs review » Closed (won't fix)

Nothing new in #5. Please, refer to #4

alex_b’s picture

#4: Ok, your call.

Forgot to mention in #3: the mere presence of FNCCK in the search path breaks the installation profile. I haven't included it in the .profile itself at all. Just FYI, doesn't change anything in the face of the argumentation of #4.

Summit’s picture

Hi, what will be the progress direction to get Formatted Number CCK integrated with feeds please?
Thanks a lot in advance for your reply!
greetings, Martijn

traxer’s picture

Status: Closed (won't fix) » Active

During the installation of Drupal, a constant MAINTENANCE_MODE is defined and set to 'install'. This can be used to test whether it is too early to check the version of the Format Number API and a warning could be issued to look at index.php?q=admin/reports/status once the installation is completed.

I'll supply a patch if you are interested (i.e. if this issue stays open long enough).