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.

Sign up for GitHub

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

Jump to bottom

Remove old PHP target #6604

Closed
wants to merge 4 commits into from
Closed

Remove old PHP target #6604

wants to merge 4 commits into from

Conversation

Simn
Copy link
Member

@Simn Simn commented Sep 22, 2017

I like @RealyUniqueName click the button.

CI fails because of something unrelated on development.

@kevinresol
Copy link
Contributor

Does this mean haxe does not support php5 anymore?

@Simn
Copy link
Member Author

Simn commented Sep 24, 2017

Yes. Users who really need PHP5 can stick to Haxe 3 for a while longer. PHP5 reaches end of life in 2019, so there would only be a few months of coverage after the Haxe 4 release anyway.

@kevinresol
Copy link
Contributor

Hmm.. that does sound like Haxe 4 is not going to release before mid 2018.

RealyUniqueName
RealyUniqueName reviewed Sep 24, 2017
View reviewed changes
src/optimization/analyzerTexpr.ml Outdated
@@ -807,7 +805,7 @@ module Fusion = struct
let el = List.map replace el in
let e2 = replace e2 in
e2,el
| Php | Cpp when not (Common.defined com Define.Cppia) && not (Common.is_php7 com) ->
| Cpp ->
let is_php_safe e1 =
Copy link
Member

Choose a reason for hiding this comment

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

This code should be removed also

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, done!

@RealyUniqueName
Copy link
Member

After merging this PR, haxe -php will stop working without -D php7. Should we merge it as-is? Or i can work on it moving everything (std, #if conditionals etc) from php7 to php and merge after that.

@Simn
Copy link
Member Author

Simn commented Sep 24, 2017

Can't we just make -php define php7 automatically?

@RealyUniqueName
Copy link
Member

We can, but it feels like a hack. I prefer to properly move everything.

@Simn
Copy link
Member Author

Simn commented Sep 24, 2017

Fine with me.

@skial skial mentioned this pull request Sep 25, 2017
Closed
@ncannasse
Copy link
Member

ncannasse commented Sep 25, 2017 via email

@RealyUniqueName
Copy link
Member

Such users have -D php7 in their hxmls. So no additional action is required from our side.

@ncannasse
Copy link
Member

ncannasse commented Sep 25, 2017 via email

@RealyUniqueName
Copy link
Member

Defining -D php7 is a requirement to use new php generator in 3.4. So they still have to add that flag to hxml of their new project. Or maybe i don't understand what you mean.

@RealyUniqueName RealyUniqueName mentioned this pull request Sep 25, 2017
Merged
@RealyUniqueName
Copy link
Member

Updated PR in #6612

@Simn Simn deleted the rip_php branch October 9, 2017 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RealyUniqueName RealyUniqueName RealyUniqueName left review comments

Assignees

@RealyUniqueName RealyUniqueName

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Simn @kevinresol @RealyUniqueName @ncannasse