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

PHP 8 Support #872

Open
ctrlaltca opened this issue Mar 1, 2023 · 14 comments
Open

PHP 8 Support #872

ctrlaltca opened this issue Mar 1, 2023 · 14 comments

Comments

@ctrlaltca
Copy link
Member

In version 8.1 php added official support from enums.
Prado makes extensive use of TEnumerable to fake enums.
Switching to native enums could make the code less weird to new developers and static analyzers.
I know it's too early to require php 8.1, so the change should be backwards compatible for the time being.

@ctrlaltca
Copy link
Member Author

Memo: Prado::using can be made able to autoload enums, too (see #876 as an example)

@ctrlaltca
Copy link
Member Author

Today prado 4.2.2 has been released supporting php 7.4, that is EOL since 4 months.
I've branched prado-4.2 that will be the base for future 4.2.x releases, still supporting php 7.
Future development will only support php 8, and i would like to target php 8.1 to get enums and intersection types.
I hope this is reasonable, since Php 8.0 is already out of active support and will be EOL-ed in about 7 months, i guess before php8-only prado will see a release.

@ctrlaltca ctrlaltca changed the title Use php native enums PHP 8 Support Apr 6, 2023
@ctrlaltca ctrlaltca pinned this issue Apr 6, 2023
@belisoful
Copy link
Member

belisoful commented Apr 6, 2023

We finally incorporated the bug fix for Cron. Thankfully.

I was hoping to get the New asset manager into the 4.2.2 release but alas. That's ok. TIFF-EXIF is taking some time to get it right/write. (read/write... that's the point of the EXIF implementation. not funny?).

There are some methods throughout that were conditionally coded for PHP 7, I specifically remember TAuthManager having a security function that needed support for older systems.

@belisoful
Copy link
Member

Man, I was hoping to get more bug fixes into this release.

@ctrlaltca
Copy link
Member Author

Don't worry, there's always time for 4.2.3

@ctrlaltca
Copy link
Member Author

#879 has been merged and now master is php8-only.
I experimented a lot with php8 enums and i have a working branch, but it turned out they are more problematic than i first thought.
This is mostly due to the fact that Prado TEnumerations are really backed by a string, while php's BackedEnums are objects and you need to use ->tryFrom() to convert from a string and ->value to convert to a string.
Converting from a string is fine since most code is supposed to use TPropertyValue::ensureEnum() and it's easy to ad support for this special case.
Having to use ->value in all the code that actually uses the enum is more problematic. Eg. a lot of code (even in the demos) contain snippets similar to:

	public function repeaterItemCreated($sender,$param)
	{
		static $itemIndex=0;
		$item=$param->Item;
		if($item->ItemType==='Item' || $item->ItemType==='AlternatingItem')
		{

In this case ItemType will be an object and you can't really compare it to a string, even if it's a string BackedEnum.

belisoful added a commit to belisoful/prado that referenced this issue Apr 17, 2023
…s of precision

From being pushed to upgrade from PHP 8.0 to PHP 8.1+ today, I moved to 8.2.  There were a few minor errors and this patches those errors.
belisoful added a commit to belisoful/prado that referenced this issue Apr 17, 2023
…me stamp) for PHP 8.2

Dropping PHP 8.0 support, ensuring 8.2 support, the $time variable was giving an implicit float to int conversion error that needed to be explicitly defined.  This was an error in PHP 8.2
belisoful added a commit to belisoful/prado that referenced this issue Apr 17, 2023
PHP 8.2 Classes must define properties.  These classes accidentally did not define their test classes.
belisoful added a commit to belisoful/prado that referenced this issue Apr 17, 2023
…ling on validation of non-existing properties in PHP 8.2
belisoful added a commit to belisoful/prado that referenced this issue Apr 17, 2023
PHP 8 supports parameters being flagged as sensitive so they do not show up in the stack trace.  While simple setters are just formality, TAuthManager::login has many moving parts and could throw an error, validation of users may throw an error in a developers application (esp DB Users), and TDBConnection -setting the password on construct- could fail on some malfunctioning class-wide behavior.
belisoful added a commit to belisoful/prado that referenced this issue Apr 17, 2023
belisoful added a commit to belisoful/prado that referenced this issue Apr 17, 2023
ctrlaltca pushed a commit that referenced this issue Apr 18, 2023
… for PHP 8.2

Dropping PHP 8.0 support, ensuring 8.2 support, the $time variable was giving an implicit float to int conversion error that needed to be explicitly defined.  This was an error in PHP 8.2
ctrlaltca pushed a commit that referenced this issue Apr 18, 2023
PHP 8.2 Classes must define properties.  These classes accidentally did not define their test classes.
ctrlaltca pushed a commit that referenced this issue Apr 18, 2023
…alidation of non-existing properties in PHP 8.2
ctrlaltca pushed a commit that referenced this issue Apr 18, 2023
PHP 8 supports parameters being flagged as sensitive so they do not show up in the stack trace.  While simple setters are just formality, TAuthManager::login has many moving parts and could throw an error, validation of users may throw an error in a developers application (esp DB Users), and TDBConnection -setting the password on construct- could fail on some malfunctioning class-wide behavior.
@belisoful
Copy link
Member

FYI. PRADO on PHP 8.2 is working without error on my instance with latest merge. @ctrlaltca, You've been doing a lot of great update work, btw. I and we all appreciate it!

I'm glad that the only 8.2 issue was basically a single implicit float => int conversion/loss of precision error (from my own hand).

@belisoful
Copy link
Member

belisoful commented Apr 21, 2023

Is there any way to automatically format all the comments to "80 + n" (until next white space)? maybe 75 or 70 as the limit depending on how it looks. I think standardizing this would be great. Preserve double spaces.

Can PHP CS Fixer buff the comments too?

I also like the format of putting 3 spaces after an @ param (or other @ item) on any text that wraps so in can be identified more easily with the prior @ item.

@ctrlaltca
Copy link
Member Author

ctrlaltca commented Apr 21, 2023

Is there any way to automatically format all the comments to "80 + n" (until next white space)?

Looks like it's not possible:
PHP-CS-Fixer/PHP-CS-Fixer#1361

@belisoful
Copy link
Member

belisoful commented Apr 21, 2023

I was considering writing a little php script that reads "/**", puts paragraphs together, chunks them, an rewrites the comments in a specified length per line until white space. Also adding a few spaces to @ items when they wrap.

We have well formatted PHP-doc. I don't see what is so difficult about this other than some people have ill formatted php-doc that might make it impossible.

@belisoful
Copy link
Member

belisoful commented May 13, 2023

Along with PHP 8 support I wanted to go through Yii2 and draw out some of their core enhancements that we should have. Our behaviors update, Event handlers supporting invokable and closure, TEventHandler, and the TExceptions update (for chaining exceptions, TExitException, and TUserException) is basically all the best stuff back ported. We have a few extra features too, like global events, dynamic events, priority based events handlers rather than append/prepend, and one-to-many TClassBehavior.

Their TExceptions is compliant with PHP's Exception whereas Prado has no actual error code just a code-message. The actual PHP Exception code doesn't fit our model (yet).

I suggest maybe overloading the "ErrorMessage_what_is_wrong" like this "error message_what_is_wrong=30" and then TException looks for the "=". If found, it pulls the error-message-code apart and we have the error code encoded into the error message code with backward compatibility.

I'd like some feedback before going ahead with such an update.

@belisoful
Copy link
Member

belisoful commented May 13, 2023

I think we could use the Yii3 VarDumper. it does closures.

@belisoful
Copy link
Member

belisoful commented May 14, 2023

What is the process for back porting, say, the the Helpers of Yii2 back to Prado? Author names are kept and added to I am presuming. What about license text? copyright yiisoft. Obviously there would need to be minor changes to make the code "Prado oriented", and Yii is the child of Prado.

So Is there a process for back porting?

Their helpers are awesome. Prado could have a "Helpers" folder in Util for converters and helpers and such. We already have the Utf8Converter and TBitHelper. TBitHelper is new and can go into Prado\Util\Helpers but what about TUtf8Converter?

Here is a list of helpers that would be great to back port:

  • BaseArrayHelper - access nested arrays
  • Our Shell code could be made into a helper, because it is. Like BaseConsoleHelper
  • BaseFileHelper - manipulating files and folders
  • BaseFormatConverter. I like BaseDateFormatConverter better.
  • BaseHTML. would require many components to change to use it. it would be useful no less
  • BaseHtmlPurifier. nice minor addition
  • BaseInflector. change pluralization of words automatically.
  • BaseIpHelper. IPv6 help
  • BaseJSON. nuance are handled.
  • BaseMarkdown. We have a markdown component but as a helper service?
  • BaseStringHelper. wow. this would be great to have.
  • BaseUrl. At least some functions are useful. There may be equals in PRADO.

They've done a lot of advanced work we can use.

They don't have the TBitHelper. :)

@ctrlaltca
Copy link
Member Author

https://www.yiiframework.com/license

Both Prado and Yii are using a BSD-3 clause license, so you are free to reuse their code as you prefer as long as you maintain their copyright notice

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

No branches or pull requests

2 participants