Skip to content

Conversation

@drupol
Copy link
Contributor

@drupol drupol commented Nov 24, 2025

Hello,

I recently discovered the project https://stagex.tools/ and out of curiosity I checked how the PHP package (https://codeberg.org/stagex/stagex/src/branch/staging/packages/core/php-zts) was built. This is how I discovered those two potential interesting patches (credits to @zeroware) that could easily be merged in the PHP distribution.

I proposed the adoption of those patches to the Nix community as well at NixOS/nixpkgs#463814

I hope the branch PHP-8.3 is the correct one, let me know if there's anything to change.

Links:

This PR:

  • Use SOURCE_DATE_EPOCH (the file datetime) inside the PHAR only when it is available.

Comment on lines +452 to +454
if (tloc) {
*tloc = ts;
}
Copy link
Contributor

@Ocramius Ocramius Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the API of this function:

  • it returns a value?
  • it overwrites a reference, if given?
  • it uses an environment variable, if given?

I'd say that a rename (for clarity) would be a good idea, and the parameter can perhaps be always gone?

Copy link
Member

@ndossche ndossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should target master as it's a new feature. It should also get a test ;)
Also this is not your code, so you should get permission from zeroware and get his proper credits in (e.g. Co-authored-by tag)


static inline time_t source_date_epoch_time(time_t *tloc)
{
const char *sde;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please merge the declarations and assignments, we no longer do C89

return PHAR_G(cached_fp)[entry->phar->phar_pos].manifest[entry->manifest_pos].offset;
}

static inline time_t source_date_epoch_time(time_t *tloc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As pointed out by Ocramius, this argument seems not useful.


tsrm_env_lock();
sde = getenv("SOURCE_DATE_EPOCH");
ts = (sde) ? strtoul(sde, NULL, 10) : time(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ts = (sde) ? strtoul(sde, NULL, 10) : time(0);
ts = (sde) ? strtoul(sde, NULL, 10) : time(NULL);

time_t ts;

tsrm_env_lock();
sde = getenv("SOURCE_DATE_EPOCH");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use php_getenv, which will do the right thing on Windows and also take care of locking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants