Typed Properties

[tacman]

Change all the annotations to typed properties, e.g.

	/**
	 * Optional title attribute for footnote links.
	 * @var string
	 */
	public $fn_link_title = "";

to

	/**
	 * Optional title attribute for footnote links.
	 */
	public string $fn_link_title = "";

[tacman]

Should we tag this as a version 2? And maybe keep it in dev? The tests pass, but I'm wondering if it'll be a problem for users who are upgrading.

[michaelbutler]

definitely will want to tag this as 2.0, and update the Readme.md to say that the latest version requires PHP 7.4 or higher, while 1.9 and below support back to 5.3.

[michelf]

That version 2 bump seems right, but after testing things now I'm a bit confused. I thought those type hints would cause errors for things like:

use Michelf\MarkdownExtra;
$mde = new MarkdownExtra;
$mde->hard_wrap = "zzz"; // should be a boolean!
echo $mde->transform(131); // should be a string!

But, unexpectedly for me, this runs fine — PHP 8.0.12 (cli) — so now I wonder where's the issue. I must be missing something.

[michaelbutler]

@michelf I'm not too certain myself, but we might need to add this to the top of the file(s) to get those to error:

<?php declare(strict_types=1);

Which sounds scary, but actually this only affects logic that happens within the file, and calls to external classes/objects/methods. In other words, enabling this WON'T cause problems if downstream usages DON'T have strict types enabled. But it does mean that we would need to be extra sure we're not doing any invalid usages within the file, and we probably would need to test it a bit more first.

https://stackoverflow.com/questions/37111470/enabling-strict-types-globally-in-php-7/37112026

[michelf]

Ok, now I see. Thank you for the link.

I'm mostly interested in the effects for users of the library at the moment so we can answer the question of whether it should be bumped to v.2. Looks like only a user declaring strict_types=1 in the file that calls or configures the Markdown parser can be affected, and only if the user is also relying on type coercion (like doing $mde->hard_wrap = 1). At first glance that sounds extremely unlikely in the same file, but rewriting it as $mde->hard_wrap = read_config_from_somewhere() makes it sound more likely. I'll give it some more thoughts.

Note that the versioning policy excludes protected members from semantic versioning, so I'm only considering the effects of type hints for the public members.

[michaelbutler]

If we don't bump to 2.0 we risk syntax errors for those who used composer require 1.* to require the library. They might accidentally upgrade and if they're on PHP 7.3 or lower it will be an unrecoverable error.

[tacman]

Yes -- like KnpMarkdownBundle, which is a Symfony wrapper for php-markdown.

…

On Mon, Nov 8, 2021 at 7:44 AM Michael Butler ***@***.***> wrote: If we don't bump to 2.0 we risk syntax errors for those who used composer require 1.* to require the library. They might accidentally upgrade and if they're on PHP 7.3 or lower it will be an unrecoverable error. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#359 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEXIQMWZALEZH4EHBFVYUDUK7A2VANCNFSM5HQZMNSA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[michelf]

Would composer actually do the upgrade without checking the PHP 7.4 requirement is satisfied?

In any case, I think this will be a v.2. It'll make it clear that the PHP requirement has changed. It might also allow some changes to the public API, if necessary.

[michaelbutler]

Good point, it might be smart enough to just skip the upgrade due to the PHP requirement. Or it could error, I don't know.

I could also address #333 for version 2 as well (adding exceptions, or at least just fixing the case where preg_* function could return false)