V2.2.1 does not work with SS3

chloehjung15 commented 3 years ago

Hi I'm trying to use this with Silverstripe 3 with version 2.2.1 but it keeps giving me Class 'phptek\\Sentry\\SentryLogWriter' not found in /MYPROJECT/mysite/_config.php error. I am using silverstripe/framework v3.7.5, PHP 7.4, silverstripe-sentry v.2.2.1 This is my code

// yml file
PhpTek\Sentry\Adaptor\SentryClientAdaptor:
  opts:
    dsn: MY_DSN

//_config.php file
SS_Log::add_writer(\phptek\Sentry\SentryLogWriter::factory(), SS_Log::ERR, '<=');

The code in _config.php file is giving me the class not found error. I tried using SentryLogger as well but no luck. In composer.json, v2.2.1 supports silverstripe framework version 3 and up. Am I missing anything? I can't use v1.1.2 as I'm using PHP 7.4

Any help will be appreciated!

phptek commented 3 years ago

You're very right! The readme is incorrect. I'm not near my laptop now but if you look in the module, there are only 5 classes. I think it should be SentryAdaptor but double check and test

phptek commented 3 years ago

If after you change to the correct path it still doesn't work, then that's a legit bug. TBH I couldn't get it working on another v3 codebase either, but I thought it was due to some conflicting config. There may be a de-facto bug here.

chloehjung15 commented 3 years ago

Yeah I think it is a bug. None of the namespaces worked. I tried adding 'autoload' in composer.json to fix the namespace issue.

...
"require-dev": {
    "phpunit/phpunit": "^5.7"
},   
"autoload": {       
    "psr-4": { 
        "PhpTek\\Sentry\\": "src/",
        "PhpTek\\Sentry\\Tests\\": "tests/"
    }
}
...

After doing this SS_Log::add_writer(PhpTek\Sentry\Log\SentryLogger::factory(), SS_Log::ERR, '<='); gave a different error. Uncaught Error: Class 'SilverStripe\\Core\\Injector\\Injector' not found in /var/www/myproject/vendor/phptek/sentry/src/Log/SentryLogger.php:55 This is because SS3 does not use a namespace for Injector class which you probably know. So instead of using SilverStripe\\Core\\Injector\\Injector, it should just be \Injector. (and the same thing with the Config class) After adding a backslash in front of Injector I got this error Uncaught Error: Call to a member function setData() on null in /var/www/myproject/vendor/phptek/sentry/src/Log/SentryLogger.php:68. I am not too sure how the $client is being set to a new RavenClient object.

Anyways I went around that issue by adding $logger->client = new RavenClient; code in the function factory() just to see if there are any other errors. And it looks like SentryLogger class might be missing addFilter() function.

PHP Fatal error: Uncaught Error: Call to undefined method PhpTek\\Sentry\\Log\\SentryLogger::addFilter() in /var/www/myproject/framework/dev/Log.php:141

This was as far as I got to. Hope everything makes sense. Wasn't too sure of the last error. I'm hoping you know something about those errors.

jonom commented 3 years ago

From what I can tell, this commit and this one are causing havok. The 2.x line is for SS4, but those commits designated it compatible with SS3.

jonom commented 3 years ago

Temporary workaround:

{
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/jonom/silverstripe-sentry.git"
        }
    ],
    "require": {
        ...
        "phptek/sentry": "dev-ss3-php7"
    },
}

phptek commented 3 years ago

@jonom I'm not sure how this change would cause the behaviour observed by the OP. Your changes are installation specific changes. If those lines were problematic, then composer wouldn't have been able to install it in the first place. Or is there something else I'm not seeing?

jonom commented 3 years ago

@phptek Composer can install it fine, but the installed code is incompatible with SS3. All the 2.x releases prior to 2.2.1 required SS4, because it's SS4 code. But 2.2.1 only requires SS3. So you get SS4 code installed on an SS3 project. At least, that's what it looks like to me 😄

jonom commented 3 years ago

P.s. My fork is branched from 1.x not 2.x

jonom commented 3 years ago

Oh, and I apparently didn't click save on an edit to my initial comment. This commit is the bigger part of the problem: https://github.com/phptek/silverstripe-sentry/commit/f536a3d2064c29d28fc2e901331254a31b58eca7

phptek commented 3 years ago

Thanks for taking the time to report. Does your branch fix the problem then? If so issue a PR and I'll tag a 2.3 release.

chloehjung15 commented 3 years ago

Thanks! I did think 2.x branch was maybe incompatible with SS4. It makes sense now. I will try @jonom's workaround later when I have time.

jonom commented 3 years ago

Does your branch fix the problem then? If so issue a PR and I'll tag a 2.3 release.

If my branch was used (probably needs some tweaking first) I think it would be a 1.1.3 release.

The 2.2.1 tag is invalid and I'm not sure that issuing a 2.2.2 tag would solve the problem. As far as I understand it Composer would still consider the 2.2.1 release when determining compatibility so would continue to install it on SS3 even if 2.2.2 is marked as requiring SS4. I'm not sure if this is appropriate but I would consider deleting the 2.2.1 tag from git and Composer (would need to manually delete from Packagist) in addition to releasing a 2.2.2 that requires SS4.

phptek commented 3 years ago

Findings

I can install 2.2.1 with composer in a framework 3.7.7 setup A-OK. I did this yesterday
I ran several versions of 2.x in a v3.77 setup, none of which worked (didn't report anything to Sentry)
I also created a branch 2.2.2-wip (based off 2.2.1) which removed any assumptions on namespacing other than for classes in the module itself, but it still didn't work.

Conclusions

SS3 doesn't work with 2.x because of the assumptions of the latter on the use of Monolog which SS3's logging system didn't use.
This means that yes, the composer requirements are indeed wrong in 2.2.x but not in supporting SS3, no - the 2.x and 3.x series were actually aimed at SS4 and now that I recall, actually represented major changes in the underlying sentry/sentry library. With hindsight, it was unnecessary to start a new release-line which confused the hell out of everyone including myself! Sorry folks, it didn't help that this itself wasn't documented properly
I'm going to go from the basics here and: Ensure 1.x works in 3.7.7 Ensure 2.x and 3.x both work OOTB with SS4.

If the above works, then I will update the composer requirements and README's for each and issue new minor release-tags for each of the 1,2 and 3.x series. I'll also point out that as SS3 is no longer fully supported, then my time is best applied to mantaining SS4 compatible releases.

Thoughts on this approach are welcome.

jonom commented 3 years ago

This means that yes, the composer requirements are indeed wrong in 2.2.x but not in supporting SS3, no - the 2.x and 3.x series were actually aimed at SS4 and now that I recall, actually represented major changes in the underlying sentry/sentry library

I think it's only 2.2.1 that is incorrect? According to Packagist, 2.2.0 and each 2.x release prior to it required SS4. That's what was really odd about the pair of commits I linked to, it changed the 2.x line composer requirements to require SS3 instead of SS4 and changed the docs to say that 2.x is for SS3, when it previously said (correctly I assume) that it is for SS4.

But yeah, plan sounds good! Probably patch release would be enough though instead of minor? Thanks for working on it!

phptek commented 3 years ago

I have created new releases:

3.0.9 (Updated README)
2.3.0 (Patched composer.json back to silverstripe/framework ^4 and same README changes)

I'll close this for now. If anyone has further problems, just create a new issue. Thanks all 👍

phptek / silverstripe-sentry

V2.2.1 does not work with SS3 #67

Findings

Conclusions