littleredbutton / bigbluebutton-api-php

Unofficial (but better) PHP API for @BigBlueButton
GNU Lesser General Public License v3.0
24 stars 12 forks source link

Inconsistent features const casing #168

Closed SamuelWei closed 5 months ago

SamuelWei commented 5 months ago

When I copied code from upstream, I used IMPORT_PRESENTATION_WITHANNOTATIONS_FROM_BREAKOUTROOMS for importPresentationWithAnnotationsFromBreakoutRooms

However the correct casing would be IMPORT_PRESENTATION_WITH_ANNOTATIONS_FROM_BREAKOUT_ROOMS

Same for IMPORT_SHARED_NOTES_FROM_BREAKOUTROOMS importSharedNotesFromBreakoutRooms

We cannot define two consts, one with the correct and one with the wrong/old syntax, as the MabeEnum\Enum doesn't allow two consts having the same value.

@FelixJacobi How should we progress?

SamuelWei commented 5 months ago

We could use an ugly workaround like this

    /**
     * @deprecated Use Feature::IMPORT_PRESENTATION_WITH_ANNOTATIONS_FROM_BREAKOUT_ROOMS instead
     */
    public const IMPORT_PRESENTATION_WITHANNOTATIONS_FROM_BREAKOUTROOMS = 'DEPRECATED-importPresentationWithAnnotationsFromBreakoutRooms';

    /**
     * @deprecated Use Feature::IMPORT_SHARED_NOTES_FROM_BREAKOUT_ROOMS instead
     */
    public const IMPORT_SHARED_NOTES_FROM_BREAKOUTROOMS = 'DEPRECATED-importSharedNotesFromBreakoutRooms';

    /**
     * Transforms the feature list to allow deprecated features and trigger a deprecation notice.
     *
     * MabeEnum\Enum only allows for unique values, so we can't have a deprecated const and a non-deprecated const with the same value.
     * This make it impossible to rename the constant and keep the old one for backward compatibility.
     * As a workaround, we are using a prefix to mark the deprecated const and trigger a deprecation
     * notice but remove the prefix before passing the value to the bbb server.
     *
     * @param array $features
     * @return array
     */
    public static function transformFeatureList(array $features): array
    {
        return array_map(function (string $feature) {
            if(str_starts_with($feature, 'DEPRECATED-')) {
                @trigger_error(sprintf('The feature "%s" is deprecated and will be removed in the next major release.', $feature), E_USER_DEPRECATED);
                return str_replace('DEPRECATED-', '', $feature);
            }
            return $feature;
        }, $features);
    }
FelixJacobi commented 5 months ago

You can just deprecate the old constants with just add @deprecated since version x.y. Use xyz instead. That should be enough, since commonly spread IDEs like PHPStorm will mark those. No need to trigger a runtime deprecation. That is only needed, when you can't mark things directly as deprecated using docs (e.g. deprecated method argument value).

SamuelWei commented 5 months ago

I thought so too and implemented it, see: https://github.com/littleredbutton/bigbluebutton-api-php/pull/169/commits/4ea2a5eeaa58e46c325691cef5642434dc79e0c5

However this gives me the following error during testing: Ambiguous enumerator values detected for BigBlueButton\Enum\Feature in bigbluebutton-api-php/vendor/marc-mabe/php-enum/src/Enum.php:401

MabeEnum uses this noAmbiguousValues function, which throws errors: https://github.com/marc-mabe/php-enum/blob/master/src/Enum.php#L413

FelixJacobi commented 5 months ago

That is the reason, why I want to get rid of these hackish fake enums as soon as possible 😄 .

Ok, I think, we can use your proposal for now. We can change that in 6.0.