Don't write API deprecations to a file

[vicdelfant]

It appears that if an API deprecation occurs, the SDK attempts to write this deprecation to a file named .last_api_deprecation_warning. This happens in the Http class and does not seem to be configurable behavior.

In our case, we (as most container-based environments) prevent write access to an application's directories except for a few selected ones, and /vendor definitely isn't one of them for obvious reasons. The file_put_contents call unfortunately doesn't have any error handling so a deprecation is now causing exceptions that then trigger our monitoring.

Would it be possible to remove this logic? If I'm being totally honest I don't see the need to write anything directly to a file; that's what libraries such as Monolog or another PSR-compliant library are meant for.

[jordansavant]

^ This
No composer library should be attempting to write to filesystem, especially a local directory. A simple deprecation warning now produces a fatal error for the entire application.

[wensonsmith]

Same problem here. I don't understand why write a file in /vendor folder. 🤯

[electricsheep]

Is there any workaround for this besides forking the library?

This causes a fatal crash and means I can't fetch orders using Order::find() - despite using the most recent API and not requesting any deprecated fields/endpoints, I still get the deprecation warning.

[electricsheep]

Answering my own question with a workaround I found:

1. Add the following to composer.json

    "autoload": {
        "psr-4": {
            "Shopify\\Clients\\": "app/Overrides/"
        },
        "exclude-from-classmap": ["vendor/shopify/shopify-api/src/Clients/Http.php"]
    },

2. Copy the Shopify implementation of the Http class to app/Overrides/ folder.
3. Remove the deprecation file writing code (below) from the logApiDeprecation function of the copied class (or modify in the way you prefer)

       $warningFilePath = $this->getApiDeprecationTimestampFilePath();

        $lastWarning = null;
        if (file_exists($warningFilePath)) {
            $lastWarning = (int)(file_get_contents($warningFilePath));
        }

        if (time() - $lastWarning < self::DEPRECATION_ALERT_SECONDS) {
            return;
        }

        file_put_contents($warningFilePath, time());

4. Fix the $version line in the request() function, since it relies on the class directory location

        $version = require dirname(__FILE__) . '/../../vendor/shopify/shopify-api/src/version.php';

You may need to run composer dump-autoload after fixing.

[github-actions]

This issue is stale because it has been open for 60 days with no activity. It will be closed if no further action occurs in 14 days.

[omolto]

+1

[mariusrusu]

+1

[nick-dnc]

+1

[chrisnetonline]

+1

[appinlet]

+1

[appinlet]

+1

[nick-dnc]

+1

[zchermit]

+1

[appinlet]

+1

[appinlet]

+1

[mkbodanu4]

Same issue since Oct 02. All the payments stopped working, so I had to comment out code in the library (file src/Clients/Http.php line 208-213) to get the app working

[appinlet]

+1

[fredden]

From what I can tell, this was fixed in #263 (3ae0ed4) by moving the temporary file into sys_get_temp_dir() instead of writing to a path within vendor/. This was released as part of version 5.0.0 of this library. (My previous comment was wrong.)

@paulomarg, I think this issue can be closed now.

[vicdelfant]

@paulomarg, I think this issue can be closed now.

It's up to the maintainers of course, but in all honesty I still don't get why a library has to write deprecations to a file. Even though it's just to the temp directory, there's still a file somewhere that can potentially grow to unknown sizes and can't be routed elsewhere.

[fredden]

there's still a file somewhere that can potentially grow to unknown sizes and can't be routed elsewhere.

The file contains only a unix timestamp.

[vicdelfant]

… which then makes me wonder why we need it at all, or at least from a userland perspective.

It's now turning into a discussion of principles and I know that I am on the annoying end of the spectrum, but the cleaner and less surprises in code (especially code provided by a company such as Shopify), the better :)

[ba-tech-gh]

It seems like an option to disable this is required, perhaps when doing Context::initialize.

The file .last_api_deprecation_warning makes it impossible to run 2 instances on the same server under different users (something we often do for prod and staging for small apps) because if one of the users creates the file, the other one will be unable to edit it due to lacking ownership.

[appinlet]

+1

[sercanvirlan]

+1

[PeterDKC]

The existence of this is so absolutely mind bogglingly absurd. We have a zoom call of 8 people trying to figure out how to get past this right now, it took us 3 days to figure out what was even happening. Because WHY, IN THE NAME OF ALL THE CHILDREN IN THE WORLD, would anyone do something so completely asinine as writing to /tmp/ from the web user out of a f@#$ing vendor/ package????????

If you don't understand why this is not a reasonable thing to do, I have a pamphlet for a high flying career in food service right here for you to peruse.

[appinlet]

+1

[Tango459]

+1