Skip to content

Convert enchant resources to opaque objects - #5577

Closed
kocsismate wants to merge 1 commit into
php:masterfrom
kocsismate:enchant-resource
Closed

Convert enchant resources to opaque objects#5577
kocsismate wants to merge 1 commit into
php:masterfrom
kocsismate:enchant-resource

Conversation

@kocsismate

Copy link
Copy Markdown
Member

Additionally, deprecate ENCHANT_MYSPELL and ENCHANT_ISPELL constants.

This PR is essentially the same as the reverted 5318971 commit, minus the OO interface.

Additionally, deprecate ENCHANT_MYSPELL and ENCHANT_ISPELL constants.
Co-authored-by: Remi Collet <remi@php.net>

/** @param resource $broker */
function enchant_broker_free($broker): bool {}
final class EnchantDict

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Could we use EnchantDictionary instead? The usage of the Dict abbreviation doesn't seem to fit PHP's recent naming strategy.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm okay either way.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I used EnchantDictionary

Comment threadext/enchant/enchant.c
RETURN_RES(dict->rsrc);
object_init_ex(return_value, enchant_dict_ce);
dict = Z_ENCHANT_DICT_P(return_value);
dict->pdict =pdict;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
dict->pdict=pdict;
dict->pdict=pdict;

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fixed this (as well as adding an upgrading notes) when committing to master


/** @param resource $broker */
function enchant_broker_free($broker): bool {}
final class EnchantDict

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm okay either way.

@kocsismate
kocsismate deleted the enchant-resource branch May 29, 2020 12:56
@kocsismate

kocsismate commented May 29, 2020

Copy link
Copy Markdown
MemberAuthor

I'm wondering if it is still ok to deprecate enchant_broker_free() and enchant_broker_free_dict() or is it something we should revert?

@nikic

Copy link
Copy Markdown
Member

@kocsismate It's okay, imho.

@carusogabrielcarusogabriel added this to the PHP 8.0 milestone May 29, 2020
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Aug 10, 2020
> Deprecate enchant_broker_set_dict_path, enchant_broker_get_dict_path
> enchant_dict_add_to_personal and enchant_dict_is_in_session
Ref: https://github.com/php/php-src/blob/c0172aa2bdb9ea223c8491bdb300995b93a857a0/NEWS#L71
> - enchant_broker_set_dict_path and enchant_broker_get_dict_path
> not available in libenchant < 1.5 nor in libenchant-2
> - enchant_dict_add_to_personal, use enchant_dict_add instead
> - enchant_dict_is_in_session, use enchant_dict_is_added instead
> - enchant_broker_free and enchant_broker_free_dict, unset the object instead
Refs:
* https://github.com/php/php-src/blob/c0172aa2bdb9ea223c8491bdb300995b93a857a0/UPGRADING#L662-L666
* php/php-src#5492
* php/php-src@66d42e9
* php/php-src#5577
* php/php-src@cd3e04d
Includes unit tests.
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Aug 10, 2020
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Aug 10, 2020
@kocsismatekocsismate mentioned this pull request Jun 18, 2024
2 tasks
Sign up for freeto 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.

3 participants

@kocsismate@nikic@carusogabriel