Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

query re: support for EOL versions of PHP #465

Closed
bapcltd-marv opened this issue Feb 2, 2020 · 34 comments
Closed

query re: support for EOL versions of PHP #465

bapcltd-marv opened this issue Feb 2, 2020 · 34 comments
Labels
enhancement This will enhance this library. question This issue is about one or more questions regarding this library.

Comments

@bapcltd-marv
Copy link
Contributor

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

@Sebbo94BY Sebbo94BY added enhancement This will enhance this library. question This issue is about one or more questions regarding this library. labels Feb 3, 2020
@Sebbo94BY
Copy link
Collaborator

@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
Copy link
Contributor Author

bapcltd-marv commented Feb 3, 2020

@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,

  • 19 have no listed php constraint
  • 5 require php:>=5.3, php:>=5.3.2, or php:>=5.3.3
  • 3 require php:>=5.4.0 or php:>=5.4.5
  • 1 requires php:~5.4|~7.0
  • 1 requires php:>=5.5.0
  • 1 requires php:^5.5|~7.0
  • 2 require php:>=5.6 or php:>=5.6.0
  • 1 requires php:^7 or php:>=7.0
  • 1 requires php:^7.1
  • 2 require php:^7.2 or php:>=7.2
  • 1 requires php:^7.3
  • 1 requires php:^7.4

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

p.s. update on 19 unlisted php constraints:

  • 13 require ~2.0, or 2.0.9, or ^2.0, (php:>=5.3)
    • 6 of which were for an older dev/released branch (these seem to be forks of the same system)
    • 1 of which has a php:>=5.5.9 dependency
    • 1 of which was for an older dev branch with a php:>=5.3.3,<7.0 constraint
  • 5 require ^3.0, (php:>=5.6)
    • 1 of which has a php:>=5.6 dependency
    • 1 of which has a php:^5.6.4 || ^7.0 dependency
    • 1 of which has a php:>=7.0.0 dependency
    • 1 of which has a php:>=7.2 dependency
    • 1 of which uses it as a dev dependency.
  • 1 requires @stable
  • 1 requires dev-master

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

@bapcltd-marv
Copy link
Contributor Author

bapcltd-marv commented Feb 5, 2020

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:

  • php:^5.6 (excludes php 7 entirely)
  • php:^5.6|^7 (excludes php 8)
  • php:>=5.6,<7.2 (excludes php 7.2+)

@Sebbo94BY
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

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

@Sebbo94BY
Copy link
Collaborator

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

@bapcltd-marv
Copy link
Contributor Author

bapcltd-marv commented Feb 28, 2020

current plan is:

    • we await new (minor) release of barbushin/php-imap
    • Drop/php 7.1 #475 gets merged into barbushin/php-imap and a new (major) release that drops support for php 5.6, 7.0, and 7.1
    • 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
    • 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
Copy link
Collaborator

@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
Copy link
Contributor Author

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

@Sebbo94BY
Copy link
Collaborator

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
Copy link
Collaborator

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

@bapcltd-marv
Copy link
Contributor Author

looks good :)

@Sebbo94BY
Copy link
Collaborator

Released 3.1.0! :)

@bapcltd-marv
Copy link
Contributor Author

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

@Sebbo94BY
Copy link
Collaborator

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

@Sebbo94BY
Copy link
Collaborator

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

@bapcltd-marv
Copy link
Contributor Author

what's the messages you're getting ?

@Sebbo94BY
Copy link
Collaborator

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: develop...411-OAuth-Support

@bapcltd-marv
Copy link
Contributor Author

bapcltd-marv commented Apr 17, 2020

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

@Sebbo94BY
Copy link
Collaborator

Sebbo94BY commented Apr 19, 2020

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
Copy link
Contributor Author

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

@Sebbo94BY
Copy link
Collaborator

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
Copy link
Contributor Author

bapcltd-marv commented Apr 20, 2020

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
Copy link
Collaborator

Sebbo94BY commented Apr 20, 2020

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 #493 as 4.0.0.

@bapcltd-marv
Copy link
Contributor Author

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
Copy link
Collaborator

Updated! Anything else, what I missed?

@bapcltd-marv
Copy link
Contributor Author

  • "FR How to archive e-mail in Gmail #1: " doesn't seem correct?
  • phpcpd & phpmnd not mentioned
  • ext-fileinfo is presently required, but it's specifically for MIME-related code so whether it can be refactored to be be forked off into a separate project is 🤷‍♂️
  • all the tests can be run by running composer run tests, as opposed to there just being phpunit tests
    • might need to modify composer.json to include the testdox flag mentioned in the readme?
  • were the travis live mailbox tests mentioned in the 3.x release ?

@Sebbo94BY
Copy link
Collaborator

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:

  • Sorted required extensions and ensured, that all required are mentioned
  • Updated "Run Tests" section

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

@Sebbo94BY
Copy link
Collaborator

Released v4.0.0. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This will enhance this library. question This issue is about one or more questions regarding this library.
Projects
None yet
Development

No branches or pull requests

2 participants