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

Set subject on admin instance when extracting translatable strings #6224

Merged
merged 1 commit into from
Jul 28, 2020

Conversation

jorrit
Copy link
Contributor

@jorrit jorrit commented Jul 24, 2020

Subject

After #6040 was added commands like php bin/console debug:translation execute methods on Admin class instances to extract translatable strings.

One of the methods is getForm, which calls configureFormFields. To tailor my forms I access $this->getSubject() in several of these methods. I hope this is not unusual or against best practices.

When the AdminExtractor calls getForm() no subject is present.

I added this code to make sure a basic model is available:

$admin->setSubject($admin->getNewInstance());

I am targeting this branch, because it is backwards compatible.

Closes #6221.

Changelog

### Fixed
- AdminExtractor sets subject created by `getNewInstance()` on Admin instance before extracting translatable strings.

VincentLanglet
VincentLanglet previously approved these changes Jul 24, 2020
@VincentLanglet VincentLanglet requested a review from a team July 24, 2020 08:25
@franmomu
Copy link
Member

What about creating a simple Admin with that configuration and then check the catalogue? To reflect the reason why this is changed.

@jorrit
Copy link
Contributor Author

jorrit commented Jul 24, 2020

I think it will take quite a bit of code to come up with a valid use case for having a subject in configureForm for instance. Do you have a suggestion?

@VincentLanglet
Copy link
Member

You mean something like ?

protected function configureFormFields(FormMapper $form)
{
    $form->add('username');
    $form->ifTrue($this->getSubject()->isNewsletterSubscribed())->add('email')->ifEnd()
}

@jordisala1991 jordisala1991 merged commit 2bebe98 into sonata-project:3.x Jul 28, 2020
@jordisala1991
Copy link
Member

Thank you @jorrit

@jorrit
Copy link
Contributor Author

jorrit commented Jul 28, 2020

Thanks for merging. I hadn't actually added a test as suggested by @franmomu. I have tried, but it is quite hard in this test class to use a concrete admin class as the methods that are called by the extractor need a lot of dependencies.

@franmomu
Copy link
Member

No problem, I'll try now with a functional test.

@@ -121,4 +121,18 @@ public function testExtractCallsBreadcrumbs(): void

$this->adminExtractor->extract([], $catalogue);
}

public function testExtractSetsSubject(): void
Copy link
Member

Choose a reason for hiding this comment

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

I saw this test and I thought that was the requested one @jorrit . Should we revert?

Copy link
Member

Choose a reason for hiding this comment

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

No, don't worry, I added one in #6237

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AdminExtractor should set subject
5 participants