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

5.0.1 - Add Support for Phalcon V5 and PHP 8 #1532

Open
wants to merge 26 commits into
base: 5.0.x
Choose a base branch
from

Conversation

jesusfreak3
Copy link

Hello!

  • Type: feature

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the Contributing Guidelines
  • I have checked that another pull request for this purpose does not exist
  • I wrote some tests for this PR

Small description of change:

Updated dev tools to work with Phalcon V5.0.1 and PHP 8.0/8.11.
Updated to use latest Phalcon migration V3 which includes Phalcon 5 and PHP 8 suport

Thanks

BeMySlaveDarlin and others added 23 commits September 18, 2021 11:18
* Fix phalcon#1479 more verbose pdo driver missing errors

* Fix phalcon#1508 devtools version showing phalcon version instead for 4.1.x

* Bump version to 4.1.1

* Fix phalcon#1515 Devtools version errors with Phalcon 5

* Adjusting dependencies for PHP 8 support

* phalcon#1455 - New code generation

* phalcon#1455 - New tests and fix old tests

* phalcon#1455 - CHANGELOG

* phalcon#1455 - Removed TODOs

* phalcon#1455 - Changed static calls

* phalcon#1455 - Added escaping namespace placeholder

Co-authored-by: Kevin Yarmak <ultimater@gmail.com>
Co-authored-by: Aziz Muzafarov <aziz.muzafarov@alg.team>
@jesusfreak3 jesusfreak3 changed the title 5.0.1 5.0.1 - Add Support Phalcon V5 and PHP 8 Jan 18, 2023
@jesusfreak3 jesusfreak3 changed the title 5.0.1 - Add Support Phalcon V5 and PHP 8 5.0.1 - Add Support for Phalcon V5 and PHP 8 Jan 18, 2023
@niden
Copy link
Member

niden commented Jan 18, 2023

@Jeckerson will check this more thoroughly.

First impressions:

  • you committed the composer.phar file - that should not be there.
  • the makefile was removed - why?
  • the docker-composer etc. were removed - also why?
  • other files were deleted (Snippet) and relevant tests - not sure that I agree with that.

Finally tests setup for Github actions fails - composer issue.

Thank you for the contribution but this needs a bit more work.

@jesusfreak3
Copy link
Author

@niden thanks for this, ill take a look. That was an accidental add of the composer.phar file, was working on it at 1:00am. The listed files you mentioned did not exist when I forked from Phalcon dev tools, ill double check what happened there, and will check the composer issue. Maybe I forgot to commit an update as composer update/install works fine locally and with my projects. Thanks for reviewing this.

@jesusfreak3
Copy link
Author

@niden i see what it is, i have everything running on php8.1, and not php8.0 so composer is failing here as config shows php8.0. I will rework this so php8.0 works as well since its still supported with security updates till end of this year.

@jesusfreak3
Copy link
Author

@niden ok, i see what happened.. I forked off 4.3.0 as I thought it was the latest stable, and that branch is missing those files you mentioned. I assume 4.2.0 is the latest stable version? It does not match master. Just trying to understand which branch I should have worked off of... 5.0.0 matches master, but its missing changes from 4.2.0, but 5.0.0 works less good then 4.2.0 in php 8. Though 4.2.0 has slight changes to make the version work for 5.0.0 but nothing else.. Ill re do this from master, blah...

@jesusfreak3
Copy link
Author

@niden is 4.3.x /v4.3.0 approved and legit? There is a lot of changes there adding a new dev tool method called task, updating some PHP standards with public const etc.. but also included all the deleting of the docker files and make files. Should I still use this, but just re add back all the deleted tests, make, docker etc? Let me know which direction I should go here so I can get this worked out.

@jesusfreak3
Copy link
Author

@niden or @Jeckerson any input here? I would like to get this finalized. I am already using this branch in my project so that I can still use dev tools and phalcon 5.0.1 and php 8.0/1 but would like to complete this. thanks

@rudiservo
Copy link
Contributor

hey @jesusfreak3, you should have made it from the 5.0.x branch, but if it matches the is should be ok in terms of commit signitures.

Let's try and fix this so we can move forward, you have done a lot of work, that is commendable, but it's really complicated to understand why in the middle of so many commits and with little to no explanation why the changes where made.

  • It seems you merged changes instead of rebase in the beginning, you need to fix that, rebase from upstream 5.0.x
  • The Makefile was delete, you still haven't explained why.
  • docker files and docker-compose are gone, we need those for independent dev and testing
  • you deleted Snippet.php and SnippetTest.php, what changes did you made?
  • ProjectAware was deleted
  • Lots of deletions in templates (ControllerBase, MainTask, etc), could you explain.

Some of these commits can be squashed, other would be nice to have their own PR so that we can understand what they fix.

For example added support for php8 and phalcon 5 does it require to delete files in templates?
The tests for Snippet.php, is it required or it's just refactoring code?

Like Niden said, it needs some work, but it is fixable, can you do that?
Make a copy of the branch before you start messing with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants