Why all exposed classes of the SDK are @internal?

[christophedebatz]

Hi,

I'm wondering why all the exposed classes and facades that Crowdin SDK offers are annotate with @Internal... ? It prevents your SDK consumers to use some useful methods.

Thanks per advance,

[andrii-bodnar]

Hi @christophedebatz,

The @internal classes and methods are not supposed to be used outside of the SDK. Also, this modifier controls what shouldn't be included in the documentation.

What methods do you want to use that are currently marked as internal?

[christophedebatz]

I understand what @Internal is supposed to do :-)

But for instance the interface ResponseErrorHandlerInterface only implemented once by Crowdin appears like developers could supply its own implementation to manage error handling the way he wants but this interface is marked as internal. Why doing an interface if this code is only implemented once by Crowdin?

/**
 * Class ResponseErrorHandler
 * @package Crowdin\Http
 * @internal
 */
interface ResponseErrorHandlerInterface
{
    /**
     * @param $data
     * @return mixed
     */
    public function check($data);
}

In addition of this question, another one that also concerns the php docblocks. In Crowdin.php entrypoint, Crowdin wrotes:

/**
 * Class Crowdin
 * @package Crowdin
 *
 * @property \CrowdinApiClient\Api\StorageApi storage
 * @property \CrowdinApiClient\Api\LanguageApi language
 * @property \CrowdinApiClient\Api\Enterprise\GroupApi group
 * @property \CrowdinApiClient\Api\ProjectApi project
 * @property \CrowdinApiClient\Api\BranchApi branch
 * @property \CrowdinApiClient\Api\TaskApi task
 * @property \CrowdinApiClient\Api\IssueApi issue
 * @property \CrowdinApiClient\Api\ScreenshotApi screenshot
 * @property \CrowdinApiClient\Api\DirectoryApi directory
 * @property \CrowdinApiClient\Api\LabelApi label
 * @property \CrowdinApiClient\Api\GlossaryApi glossary
 * @property \CrowdinApiClient\Api\MachineTranslationEngineApi machineTranslationEngine
 * @property \CrowdinApiClient\Api\StringsExporterSettingApi stringsExporterSetting
 * @property \CrowdinApiClient\Api\StringTranslationApi stringTranslation
 * @property \CrowdinApiClient\Api\StringCommentApi stringComment
 * @property \CrowdinApiClient\Api\Enterprise\UserApi|\CrowdinApiClient\Api\UserApi user
 * @property \CrowdinApiClient\Api\Enterprise\VendorApi vendor
 * @property \CrowdinApiClient\Api\Enterprise\WorkflowTemplateApi workflowTemplate
 * @property \CrowdinApiClient\Api\Enterprise\WorkflowStepApi workflowStep
 * @property \CrowdinApiClient\Api\FileApi|\CrowdinApiClient\Api\Enterprise\FileApi file
 * @property \CrowdinApiClient\Api\Enterprise\ReportApi|\CrowdinApiClient\Api\ReportApi report
 * @property \CrowdinApiClient\Api\SourceStringApi sourceString
 * @property \CrowdinApiClient\Api\TranslationMemoryApi translationMemory
 * @property \CrowdinApiClient\Api\WebhookApi webhook
 * @property \CrowdinApiClient\Api\TranslationApi translation
 * @property \CrowdinApiClient\Api\TranslationStatusApi translationStatus
 * @property \CrowdinApiClient\Api\DistributionApi distribution
 * @property \CrowdinApiClient\Api\Enterprise\TeamApi team
 * @property \CrowdinApiClient\Api\Enterprise\TeamMemberApi teamMember
 * @property \CrowdinApiClient\Api\BundleApi bundle
 * @property \CrowdinApiClient\Api\NotificationApi|\CrowdinApiClient\Api\Enterprise\NotificationApi notification
 * @property \CrowdinApiClient\Api\OrganizationWebhookApi organizationWebhook
 * @property \CrowdinApiClient\Api\ReportArchiveApi|\CrowdinApiClient\Api\Enterprise\ReportArchiveApi reportArchive
 */
...

I think the "$" has been forgotten as the variable prefix. Unfortunately, stan and psalm do not recognize the virtual properties and we have the obligation to drop some suppress warning to silent the errors like this:

/** @psalm-suppress UndefinedMagicPropertyFetch */
            $branch = $this->crowdin->branch->create($projectId, ['name' => $name, 'title' => $title]); /* @phpstan-ignore-line */

[andrii-bodnar]

@christophedebatz thanks for the details!

I agree about the ResponseErrorHandlerInterface, I think we can remove the @internal there.

I think the "$" has been forgotten as the variable prefix.

Hmm, probably. Would you like to submit a PR? We are happy to accept contributions 🙂

[andrii-bodnar]

I think the wrong return type of the __get method could also be a reason for the UndefinedMagicPropertyFetch error.

[christophedebatz]

And what about ModelCollection and Collection that are @internal but returned by all API list method. Does this seem normal to you?

[andrii-bodnar]

And what about ModelCollection and Collection that are @internal but returned by all API list method. Does this seem normal to you?

Could you please show a real-world example of how it could be used?

[christophedebatz]

For sure, see below:

Use:

Lint warnings:

You are returning some DTO that developers can use but these DTO are @internal. So there methods are not available. So the only workaround is to not store the ModelCollection in our code and to use directly the reading methods. But I cannot see why disallow DTO manipulation to developers.

[andrii-bodnar]

@christophedebatz thank you! I think in these cases it would also be better to get rid of the @internal modifier.