Some of us are trying to use composer, gah! ;)

gelf-php available from composer, autoloads by classmap.

Patch to follow.

CommentFileSizeAuthor
#15 gelf-dont-require-libraries-1664282-15.patch7.21 KBglennpratt
#9 gelf-dont-require-libraries-1664282-8.patch7.4 KBglennpratt
#8 composer.patch7.56 KBrobloach
#6 composer.patch7.54 KBrobloach
#5 gelf-dont-require-libraries-1664282-6.patch6.55 KBglennpratt
#4 gelf-dont-require-libraries-1664282-5.patch6.43 KBglennpratt
#3 gelf-dont-require-libraries-1664282-3.patch6.43 KBglennpratt
#1 gelf-dont-require-libraries-1664282-1.patch6.15 KBglennpratt

Comments

glennpratt’s picture

glennpratt commented
Status: Active » Needs review
StatusFileSize
new gelf-dont-require-libraries-1664282-1.patch6.15 KB

There we go.

patcon’s picture

patcon commented
Issue tags: +Composer

tagging

glennpratt’s picture

glennpratt commented
StatusFileSize
new gelf-dont-require-libraries-1664282-3.patch6.43 KB

Added composer.json

glennpratt’s picture

glennpratt commented
StatusFileSize
new gelf-dont-require-libraries-1664282-5.patch6.43 KB

Fixed composer.json with Rob Loach's help.

glennpratt’s picture

glennpratt commented
StatusFileSize
new gelf-dont-require-libraries-1664282-6.patch6.55 KB

Sorry, forgot to stage the changes.

robloach’s picture

robloach
he/him
commented
StatusFileSize
new composer.patch7.54 KB

Cleaned up gelf_require() a bit, and made the status report use it. Checks gelf/vendor/autoload.php and sites/all/vendor/autoload.php (or sites/example.com/vendor/autoload.php if sites/example.com/modules/gelf).

glennpratt’s picture

glennpratt commented
Status: Needs review » Needs work 
+++ b/gelf.installundefined
@@ -13,8 +13,7 @@ function gelf_requirements($phase) {
+    if (!gelf_require()) {

Good catch!

+++ b/gelf.moduleundefined
@@ -87,10 +83,50 @@ function gelf_form_system_logging_settings_alter(&$form, $form_state) {
+  return $loaded = FALSE;

Even if the class is loaded, this will cause subsequent calls to this function to fail because the block above is bypassed.

I would lean toward just class_exists. Though I don't have a benchmark, I imagine the static cache would be better used preventing a call to this function entirely in gelf_watchdog().

robloach’s picture

robloach
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new composer.patch7.56 KB

Whoops!

    }
    $loaded = FALSE;
  }
  return $loaded;
glennpratt’s picture

glennpratt commented
Status: Needs review » Needs work
StatusFileSize
new gelf-dont-require-libraries-1664282-8.patch7.4 KB

I removed the static cache because of that hiccup in testing for now, we got by calling core api, libraries API and require every call before :)

This patch actually tested, unlike my others ;)

glennpratt’s picture

glennpratt commented
Status: Needs work » Needs review

Ahh, cross-posted.

glennpratt’s picture

glennpratt commented
Status: Needs review » Needs work 
+++ b/gelf.moduleundefined
@@ -87,10 +83,47 @@ function gelf_form_system_logging_settings_alter(&$form, $form_state) {
+  $sitepath = dirname(dirname($modulepath));

I realized this wouldn't work in our case because we install contrib modules into a contrib directory - I don't believe we are the only ones doing that.

Perhaps conf_path() is a better choice?

msonnabaum’s picture

msonnabaum commented

I'm not sure we should support /sites/all/vendor. Is that a documented convention? Feels arbitrary to me.

glennpratt’s picture

glennpratt commented

I feel like we're trying to come up with a convention here. My argument for searching /profile/%name%/vendor is the profile is where my composer.json lives (and makefile and functional tests, etc). I think that is a good pattern, however your active site could include code from the profile and from /sites/all and /sites/default (or whatever site).

So my current thought is the active site (conf_dir()) is the most rational place to look. sites/default/vendor for example.

I think we're going to need to try this stuff out and build tools for it - it might just be too much for this issue.

NOTE: To clarify, I mean look for the autoloader in sites/default/vendor or similar, not composer.json.

msonnabaum’s picture

msonnabaum commented

Well, my thought is, if you have your vendor in /profile or /sites/all, then you should include that somewhere else, like maybe your settings.php or something. If you've already included it, we check with class_exists, it's autoloaded, and we dont need to look for a vendor dir local to the module directory.

That seems reasonable to me and covers the all use cases without trying to establish new conventions.

glennpratt’s picture

glennpratt commented
StatusFileSize
new gelf-dont-require-libraries-1664282-15.patch7.21 KB

I totally agree, though in retrospect I can see how I may have led the conversation astray - as long as the class_exists comes first I'm happy.

robloach’s picture

robloach
he/him
commented
Status: Needs work » Needs review

Looks pretty good to me!

msonnabaum’s picture

msonnabaum commented
Status: Needs review » Reviewed & tested by the community

Fix the missing quote in the composer example and it's good.

glennpratt’s picture

glennpratt commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 7.x-1.x. Thanks.

http://drupalcode.org/project/gelf.git/commitdiff/4389d96300b2e5e76154b4...

Moving to 6.x