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

Declare ext/tidy properties #8515

Merged
merged 2 commits into from
May 26, 2022
Merged

Declare ext/tidy properties #8515

merged 2 commits into from
May 26, 2022

Conversation

kocsismate
Copy link
Member

No description provided.

@kocsismate kocsismate requested review from iluuu1994, cmb69 and Girgias May 8, 2022 09:21
Girgias
Girgias approved these changes May 8, 2022
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

LGTM, but where the properties always readonly?

@kocsismate
Copy link
Member Author

LGTM, but where the properties always readonly?

No, they weren't before.. So this is up for debate whether we want/can make them readonly. I think it would make sense to add the readonly modifier for them.

@cmb69
Copy link
Member

cmb69 commented May 8, 2022

It seems that the properties were writable before, but changing them would not have the desired effect, e.g.

<?php

$html = <<<HTML
<body onload="foo">asd</body>
HTML;
$tidy = new tidy();
$tidy->parseString($html);
$body = $tidy->body();
var_dump($body->attribute);
$body->attribute = [];
var_dump($body->attribute);
echo $body;
array(1) {
  ["onload"]=>
  string(3) "foo"
}
array(0) {
}
<body onload="foo">
asd
</body>

As such, making the properties read-only is a step in the right direction, but still would constitute some kind of BC break. A cleaner solution would be to deprecate writing to the properties first (and to later make them read-only), but that looks like overkill.

Maybe write to internals about your proposal.

@cmb69
Copy link
Member

cmb69 commented May 9, 2022

FTR: mailing list discussion

@kocsismate
Copy link
Member Author

I'm going to merge this PR at the end of the next week, unless any resistance or issue emerges

@ramsey ramsey added this to the PHP 8.2 milestone May 21, 2022
@kocsismate
Copy link
Member Author

@ramsey How much do you think we should wait with the merge?

@cmb69
Copy link
Member

cmb69 commented May 25, 2022

IMO, go ahead and merge. :)

@kocsismate kocsismate mentioned this pull request May 25, 2022
Open
@ramsey
Copy link
Member

ramsey commented May 25, 2022

Agreed! All I've heard are crickets. 😄

@kocsismate kocsismate merged commit aeb4aca into php:master May 26, 2022
@kocsismate kocsismate deleted the tidy-properties branch May 26, 2022 06:26
@ramsey
Copy link
Member

ramsey commented May 26, 2022

I think this warrants a note in the NEWS file and perhaps UPGRADING.

@kocsismate
Copy link
Member Author

Yes, I added an upgrading entry in df77fee, but AFAIK we don't add NEWS entries for such technical changes.

@ramsey
Copy link
Member

ramsey commented May 26, 2022

Sounds good to me. I missed seeing that commit. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Girgias Girgias Girgias approved these changes

@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

@cmb69 cmb69 Awaiting requested review from cmb69

Assignees
No one assigned
Labels
Extension: tidy
Projects
None yet
Milestone
PHP 8.2
Development

Successfully merging this pull request may close these issues.
4 participants
@kocsismate @cmb69 @ramsey @Girgias