barbushin / php-imap

Manage mailboxes, filter/get/delete emails in PHP (supports IMAP/POP3/NNTP)
MIT License
1.66k stars 458 forks source link

query re: support for EOL versions of PHP #465

Closed bapcltd-marv closed 4 years ago

bapcltd-marv commented 4 years ago

I was just wondering what the plans are for supporting php 5.6/7.0/7.1, given that they're all EOL?

Sebbo94BY commented 4 years ago

@barbushin initially wanted to keep the support for these PHP versions.

In my opinion we can remove the support for all unsupported PHP versions: https://www.php.net/supported-versions.php

So I would only support active PHP versions such as PHP 7.2 and newer/higher.

We also somehow need to force users to update their old stuff. When they always see, that the software is still supported with an older version, they will never update their system as it's working.

Some coding improvements of this library are even only possible, when we remove the support for older versions, so we should think about removing support for EoL PHP versions.

Any feedback / suggestions? @bapcltd-marv @barbushin

bapcltd-marv commented 4 years ago

@Sebi94nbg re: code improvements, see:

If you wanted to jump straight to php:^7.2, you'd get strongly-typed parameters, strongly-typed returns, object type hints & void returns

Our downstream fork jumps straight to php:^7.4, purely because of not needing to support older versions of PHP.

Of the dependant packages listed in packagist,

You could consider supporting a pair of ^5.6|<7.2 & a ^7.2 branches, but that would create additional overhead that might not be worth the additional effort?

p.s. advantages of jumping to 7.4 are the typed properties & being able to reasonably include a composer.lock file in the repo.

Sebbo94BY commented 4 years ago

Ok, let's remove the no longer supported PHP versions.

Jumping directly to PHP 7.4 supported code, would be awesome, but I want to keep the support for PHP 7.2 and 7.3.

bapcltd-marv commented 4 years ago

I'd imagine you'd want to put a new 3.x release out before I PR the appropriate branch for a 4.x release?

by the sounds of it, you'd be wanting the "drop/php-7.1" branch (not PRing yet due to recurring habit of rebasing it/tweaking the patch set).

re: "forcing" users to upgrade, I'm not sure I have any relevant experience to comment appropriately beyond support policies on 3.x/4.x.

bapcltd-marv commented 4 years ago

p.s. update on 19 unlisted php constraints:

(may have miscounted one of the 2.x/3.x counts)

bapcltd-marv commented 4 years ago

re: "forcing" users to upgrade, I'm not sure I have any relevant experience to comment appropriately beyond support policies on 3.x/4.x.

further thoughts:

Sebbo94BY commented 4 years ago

Mhmm, I would exclude PHP 8 in this case for now.

Excluding PHP 7.2+ is no good idea at all and all PHP 7 versions as well not.

bapcltd-marv commented 4 years ago

Excluding PHP 7.2+ is no good idea at all and all PHP 7 versions as well not.

the idea behind php:>=5.6,<7.2 for 3.x & php:^7.2 for 4.x would be forcing people to upgrade- the negative impact being if 3.x was a dependency of one dependency and 4.x was a dependency of another dependency, you'd run into a problem.

bapcltd-marv commented 4 years ago

you'd run into a problem.

practical example: symfony/console- it's common for developer tooling to use symfony/console to handle CLI tasks, but some newer releases of tools don't also use newer versions of the console package, e.g.: 1) latest tagged releases of maglnet/composer-require-checker & vimeo/psalm both support symfony/console 5.x 2) the latest tagged release of povils/phpmnd supports a max of symfony/console 4.x

which leads to a lower constraint being applied to maglnet/composer-require-checker and occasionally needing to manually constrain symfony/console (requiring and then removing it if it's not used directly).

tl:dr; lowering the upper bound of php supported for 3.x may force uses to upgrade, and could lead to user frustration.

Sebbo94BY commented 4 years ago

It's always kinda complicated to avoid frustating users. :(

Sebbo94BY commented 4 years ago

@bapcltd-marv what's the plan now? When can we publish all your improvements with which version?

bapcltd-marv commented 4 years ago

current plan is: 1) [x] we await new (minor) release of barbushin/php-imap 2) [x] oauth gets dropped from develop branch 3) [x] I rebase #475 & bring it out of draft 4) [x] #475 gets merged into barbushin/php-imap and a new (major) release that drops support for php 5.6, 7.0, and 7.1 5) [ ] at some point in the future, give me a nudge to PR in the changeset for dropping support for php 7.2 and 7.3 6) [ ] at some point in the distant future, give me a nudge in case I've dropped php 7 entirely.

p.s. see also optionally providing guidance for getting some of the private tests into the repo in some form or another.

Sebbo94BY commented 4 years ago

@bapcltd-marv everything ready in the develop branch? If yes, I would now create the minor release 3.1.0, that we can continue with your other mentioned points. :)

bapcltd-marv commented 4 years ago

@Sebi94nbg are we doing anything with integrating your private tests into the repo?

Sebbo94BY commented 4 years ago

The most of these tests are already in the repo. I just double-checked them before all the time since I never was sure, that the repo tests were working as expected.

So, I would now merge develop into master and release 3.1.0 then, when you agree.

Sebbo94BY commented 4 years ago

@bapcltd-marv did I miss anything regarding what changed? #491

bapcltd-marv commented 4 years ago

looks good :)

Sebbo94BY commented 4 years ago

Released 3.1.0! :)

bapcltd-marv commented 4 years ago

if you can fork & remove oauth from develop, I can get to rebasing #475 once I'm done watering the office plants.

Sebbo94BY commented 4 years ago

Sure, I'll do that in around one hour. I'll get some ice cream first. :)

Sebbo94BY commented 4 years ago

I've unfortunately some trouble with pulling/pushing from/to Git from my local system. I need to investigate this issue further. :'(

bapcltd-marv commented 4 years ago

what's the messages you're getting ?

Sebbo94BY commented 4 years ago

Sorry for the delay, I am still moving to my new flat and that will last until the end of May due to COVID-19.

I could now partitially fix the issue with Git / VSCode.

It was always reporting, that the SSH key couldn't be loaded or used for authentication or that the host key could not be verified. At the moment, I can run git pull and git push from the VSCode CLI, but the UI feature to sync everything with one click is still not working - it reports the error Git: Host key verification failed..

I've now removed the OAuth code from the develop branch (commit f85a74) and created a new branch based on the develop branch, which includes the OAuth code: https://github.com/barbushin/php-imap/compare/develop...411-OAuth-Support

bapcltd-marv commented 4 years ago

~currently awaiting travis to finish on the rebase.~ made the mistake of running update commands in a php-7.4 window ¬_¬

Sebbo94BY commented 4 years ago

Ok, I've merged your PR re dropping old PHP support. And also implemented the code change from the other PR regarding this Base 64 decoding.

Are we ready to release 3.2.0? Everything updated? README, composer.json, ...?

It would be nice, when we could have a successful build: https://travis-ci.org/github/barbushin/php-imap/jobs/676976779 :D

bapcltd-marv commented 4 years ago

dropping php versions should be a major release, not a minor release, i.e. 4.0.0

Sebbo94BY commented 4 years ago

Yeah, you're right. Should be a major release as it drops old PHP support.

Any idea, how we can fix this Travis CI issue? Is anything else todo except the fix for Travis CI?

bapcltd-marv commented 4 years ago

travis was down for maintenance earlier, couldn't see the error; ~having a look now.~ ah yeah, just typecast it like the other cases.

Sebbo94BY commented 4 years ago

I've added that typecast, thanks.

I've added a table regarding the supported PHP versions to the README: https://github.com/barbushin/php-imap/blob/develop/README.md Would you change that or is this fine?

develop build is fine now: https://travis-ci.org/github/barbushin/php-imap/builds/677385039 :)

If I didn't miss anything, I'll publish this PR https://github.com/barbushin/php-imap/pull/493 as 4.0.0.

bapcltd-marv commented 4 years ago

the readme isn't entirely accurate re: ext-iconv; both it and ext-mbstring are presently required in composer.json- installation will fail without them.

Sebbo94BY commented 4 years ago

Updated! Anything else, what I missed?

bapcltd-marv commented 4 years ago
Sebbo94BY commented 4 years ago

Interesting. No clue, where I got this FR1-thing from. Can't find it anymore. :o

were the travis live mailbox tests mentioned in the 3.x release ?

Nope, weren't mentioned. See https://github.com/barbushin/php-imap/releases. I've added this as information to the changelog.

I've updated the README:

When the build is not failing, I'll then release the new version 4.0.0. :)

Sebbo94BY commented 4 years ago

Released v4.0.0. :)