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.

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

IBX-7409: Changed Content Type to content type #316

Merged
merged 11 commits into from
Jan 22, 2024

Conversation

Gengar-i
Copy link
Contributor

@Gengar-i Gengar-i commented Jan 3, 2024

Question Answer
JIRA issue IBX-7409
Type bug
Target Ibexa version v4.6
BC breaks no

Changed Content Type to Content type

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (main for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ibexa/engineering).

@Gengar-i Gengar-i changed the title IBX-7409: Changed Content Type to Content type IBX-7409: Changed Content Type to content type Jan 8, 2024
Copy link
Contributor

@mikadamczyk mikadamczyk left a comment

Choose a reason for hiding this comment

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

In translation configuration we have

        $container->prependExtensionConfig('jms_translation', [
            'configs' => [
                'ibexa_core' => [
                    'output_format' => 'xlf',
                ],
            ],
        ]);

why there is a change of extension from xlf to xliff?

the only valid way to extract translations is to run php bin/console translation:extract --config=ibexa_core

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Given the extensive diff I'm not sure if changing PHPDoc comments (especially behat ones, but all in general) is such a good idea. Not saying no, because the changes are for the most part correct, with some remarks I posted below.

What I'm worried about is 1st party compatibility of CI tests. We probably should think with @mnocon how to test this against all packages, because I have a feeling we might test in the other packages against specific case of exception messages. Technically changing exception message between minor releases is a BC break, but it rather affects automated tests only. As long as we won't break our CI, it's fine.

src/lib/Persistence/Cache/Readme.md Outdated Show resolved Hide resolved
src/contracts/Repository/ContentTypeService.php Outdated Show resolved Hide resolved
src/contracts/Repository/Values/Content/ContentInfo.php Outdated Show resolved Hide resolved
src/lib/Repository/Validator/TargetContentValidator.php Outdated Show resolved Hide resolved
tests/lib/Persistence/Legacy/TransactionHandlerTest.php Outdated Show resolved Hide resolved
@Gengar-i Gengar-i force-pushed the ibx-7409-changed-content-type-label branch from 66cab32 to 0f11586 Compare January 9, 2024 15:31
@Gengar-i Gengar-i requested a review from mikadamczyk January 10, 2024 09:36
@alongosz
Copy link
Member

@Gengar-i translation extraction issue fix has been merged into main and you can now rebase & regenerate translations.

@Gengar-i Gengar-i force-pushed the ibx-7409-changed-content-type-label branch from bb98880 to 3395daa Compare January 10, 2024 13:50
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

+1 but it requires CI (unit, integration) sanity checks for packages which require core (almost all of them) but were not changed for this ticket.

@Nattfarinn Nattfarinn force-pushed the ibx-7409-changed-content-type-label branch from 3395daa to 7cf9a6d Compare January 17, 2024 09:14
Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
No data about Coverage
2.7% Duplication on New Code

See analysis details on SonarCloud

@Nattfarinn Nattfarinn merged commit 2ac0538 into main Jan 22, 2024
21 checks passed
@Nattfarinn Nattfarinn deleted the ibx-7409-changed-content-type-label branch January 22, 2024 13:54
Sign up for free to 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.

9 participants