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

[WIP] [RFC] Embedded crud in forms #6384

Draft
wants to merge 1 commit into
base: 4.x
Choose a base branch
from

Conversation

psihius
Copy link
Contributor

@psihius psihius commented Jul 25, 2024

Note: this is a WIP and a Request For Comments aka feedback. This needs a lot more development, especially on making things work via AJAX and having a dedicated "embedded" layout, because right now it does not have as smooth functionality as it needs to. Also, a bunch of JS is commented out because I did not get to it yet. Filters are gonna be a major headache and probably would need improvements overall. This might turn into an epic of its own.

This is the ability to embed a whole data grid "index" page into a form via the EmbedField::new('relation')

This was based on #3543 PR, but a lot has changed since 3.x days and that PR needed a full re-work and bigger changes to the EA.

  • All actions happen by regular navigation - except paging nothing else is using AJAX and/or modals.
  • All actions use referrers, barring unusual circumstances, they should redirect back to the page you were on, anchors should work too ( # )
  • I have not tried adding filters at all, nor is JS code handling them anyway (commented out)
  • No handling for batched actions
  • Things that work: index page (grid), paged navigation, add, edit, delete, custom actions.

Lots to work on:

  • Search
  • Sorting
  • Filters
  • Batched actions
  • AJAX-based deleting
  • Modal based add/edit/custom actions
  • Move JS into it's own file to be included according to EA way

All feedback, patches and discussion are welcome. There's a wide field of discussion on how to approach this and implement it - I am thinking that we probably should have a dedicated embedded_layout form and have a dedicated template for the embedded grid rather than doing a single block render like it's doing now.

@psihius psihius changed the title [WIPEmbedded crud [WIP] [RFC] Embedded crud in forms Jul 25, 2024
@psihius psihius force-pushed the embedded-crud branch 4 times, most recently from fd78221 to f379763 Compare July 25, 2024 10:26
@javiereguiluz javiereguiluz added this to the 4.x milestone Jul 25, 2024
@psihius psihius marked this pull request as draft July 25, 2024 18:37
@Seb33300
Copy link
Contributor

@psihius can you please share a screenshot of how i looks like when we embed a data table in a form?

@psihius
Copy link
Contributor Author

psihius commented Jul 30, 2024

@psihius can you please share a screenshot of how i looks like when we embed a data table in a form?
Screenshot from 2024-07-30 19-17-25

Comment on lines +135 to +165
/** @var array|null $embedContext */
$embedContext = $context->getRequest()->query->all('embedContext');
if (\array_key_exists('mappedBy', $embedContext)) {
$filterProperty = $embedContext['mappedBy'];
$filterValue = $embedContext['embeddedIn'] ?? null;
if (null !== $filterValue) {
// Use the parameter conversion capabilities of Doctrine
$metadata = $queryBuilder->getEntityManager()->getClassMetadata($context->getEntity()->getFqcn());
$relatedEntityFqcn = $metadata->getAssociationTargetClass($filterProperty);
$relatedEntityMetadata = $queryBuilder->getEntityManager()->getClassMetadata($relatedEntityFqcn);
$type = $relatedEntityMetadata->getTypeOfField($metadata->getSingleIdentifierFieldName());
if (Type::hasType($type)) {
$doctrineType = Type::getType($type);
$platform = $queryBuilder->getEntityManager()->getConnection()->getDatabasePlatform();
$filterValue = $doctrineType->convertToDatabaseValue($filterValue, $platform);
}
$rootAlias = current($queryBuilder->getRootAliases());
$associationMapping = $metadata->getAssociationMapping($filterProperty);
if (in_array($associationMapping['type'], [ClassMetadata::ONE_TO_MANY, ClassMetadata::MANY_TO_MANY], true)) {
$queryBuilder->andWhere(':filterValue MEMBER OF '.sprintf('%s.%s', $rootAlias, $filterProperty))
->setParameter('filterValue', $filterValue);
} else {
$queryBuilder->andWhere(sprintf('%s.%s', $rootAlias, $filterProperty) . ' = :filterValue')
->setParameter('filterValue', $filterValue);
}
}
$field = $fields->getByProperty($filterProperty);
if ($field instanceof FieldDto) {
$fields->unset($field);
}
}
Copy link
Contributor

@Seb33300 Seb33300 Jul 30, 2024

Choose a reason for hiding this comment

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

Can you move this into a separate method to keep the index action method simple to understand?
Not sur of what's the best between creating a new createEmbededIndexQueryBuilder() or something like filterEmbededQueryBuilder()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving it is no problem, as I said - this is a rough draft that needs lots of work :)

As for the second point - maybe creating dedicated embedded(index|detail|edit|new|delete) actions or even rolling a EmbeddedAbstractController might be a better way of doing it, but then you have to maintain similar code. So the question is - what would be a better approach to refactor here so it is better long term? Those action methods already are pretty sizeable and adding to them might be a bit overkill :)

Comment on lines +26 to +33
.embed-spinner {
display: none;
top: 50%;
left: 50%;
margin-top: -1rem;
margin-left: -1rem;
z-index: 10;
}
Copy link
Contributor

@Seb33300 Seb33300 Jul 30, 2024

Choose a reason for hiding this comment

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

You should be able to remove this by adding position-absolute top-50 start-50 translate-middle to your embed-spinner
See https://getbootstrap.com/docs/5.2/utilities/position/#center-elements

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.

3 participants