Problem/Motivation
Core entity forms are currently very similar in their basic functionality but each entity type replicates more or less the same code to implement more or less the same logic. This is true for the main create/edit forms as for the other operations such as the deletion/deletion confirmation forms. This unnecessarily bloats the core codebase and makes maintaining it more difficult. Moreover changes to the common logic need to be manually propagated to all core entity types and contrib has no way to reuse the logic implemented by core entity forms.
Additionally it's impossible to tell whether a form is an entity form in a reliable way when altering it, because there is no standardization enforced among them, just some guideline. This makes handling them in a generic fashion really difficult.
Proposed resolution
By introducing a common entity form controller based on the Entity Property API we will able to generate a basic entity form without needing any additional code from the modules defining the entities. However those will be able to subclass the entity form controller and perform any needed alteration to its behavior.
An entity form controller is an object responsible for handling the entity form workflow: from building the form to performing validation and submission tasks. The base implementation provides the common logic and will exploit the Entity Property API to generalize most of its tasks, so that only minimal custom logic will need to be provided in the entity-specific subclasses.
Entity form controllers are defined in the entity info as storage ones, but they differ from those in the sense that an entity can have multiple form controllers, each one responsible for managing a different operation form. This way we can have, for instance, different controllers for the regular create/edit form and the deletion/deletion confirmation ones. Regardless of the operation, the controller defines standard methods to build the entity from the submitted values and retrieve it from the form state, thus providing a more consistent and reliable API to code altering the entity form workflow. AAMOF the controller itself is stored into the form state and can be retrieved by any form handler/callback. This is also an easy way to spot if we are dealing with an entity form.
The current solution performs a few small tweaks to the Form API, which are @chx-approved, to allow for any callable to become a form handler/callback. This way class methods can be used as form handlers and overridden by subclasses, which can customize the form submission workflow to fit their needs or just get the default behavior. By default different callbacks are assigned to the various form actions, thus allowing the controller to provide separately overridable methods corresponding to them. This way subclassess can decide to customize only the validation/submission handlers they need to.
Remaining tasks
Here is a list of follow up issues. The nested ones may or may not deserve a dedicated issue, we will see while working on the main one.
- #1728780: Build and prepare a basic entity form through the Entity Property API
- #1728786: Factor up and consolidate form actions
- #1728788: Introduce hooks for altering, entity-building, validating and submitting an entity form
- Remove global validation handlers
- #1696648: [META] Untie content entity validation from form validation [Introduce a common validation workflow for entity forms exploiting entity validation]
- Move
form_state_values_clean()
intoEntityFormController::validate()
- Move
- #1728796: Introduce a common submission workflow for entity forms
- Factor up and generalize action-specific submit handlers
- Remove form rebuild from vocabulary delete action
- Make user profile cancellation the regular user delete action
- Move entity_form_submit_build_entity() into the form controller.
- Factor up and generalize action-specific submit handlers
- #1498874: Provide language awareness to entity forms (introduce the form language concept)
- #1616930: Fix the field_test entity type [Add an
EntityTestFormController
for field test] - #1757232: Improve test coverage for the entity form controller
- #1728804: Introduce (Content)EntityDeleteForm and children to handle entity deletions
- #1728808: Provide more context to entity form controllers
- #1728812: Prevent entity form controllers from being cached twice (in $form and $form_state)
- #1728816: Ensure multiple entity forms can be embedded into one form
- #1728818: Clarify $operation and the relationship to form modes (as a counterpart of view modes)
- #1454730: Allow OO methods as FAPI / render API #callbacks [Make sure all FAPI callbacks can point to objects]
List available also at:
http://drupal.org/project/issues/search/drupal?issue_tags=Entity%20forms
User interface changes
No UI change.
API changes
The form state build info support a new 'callback'
key that can be used to explicitly set a form builder callback. Moreover now any callable can be used as a form handler/callback.
The recommended way to get the entity being operated on is no longer $form_state[$entity_type]
(or similar) but $form_state['controller']->getEntity($form_state)
.
Some new API functions have been introduced:
entity_form_controller()
can be used to instantiate a new form controller class for the given operationentity_form_state_defaults()
provides a form state stub with the build info properly populated to build an entity formentity_form_id()
computes the form id for the given entity type/operation coupleentity_get_form()
returns a rendered entity form for the given operationentity_form_submit()
submits an entity form with the given form state
Comment | File | Size | Author |
---|---|---|---|
#125 | entity_forms-1499596-125.interdiff.do_not_test.patch | 421 bytes | plach |
#125 | entity_forms-1499596-125.patch | 198.44 KB | plach |
#123 | entity_forms-1499596-123.interdiff.do_not_test.patch | 10.07 KB | plach |
#123 | entity_forms-1499596-123.patch | 198.44 KB | plach |
#106 | entity_forms-1499596-106.interdiff.do_not_test.patch | 752 bytes | plach |
Comments
Comment #1
dawehnerJust started and looking forward to do some work during the flight.
Comment #2
plachActually I already have a semi-working patch. I forgot to assign this to myself.
Comment #3
plachHere is a first attempt. The goal of this patch is providing the basic infrastructure, AAMOF the form builder/handler functions are not actually migrated to the new system. This in itself involves a deep refactoring that might deserve its own issue. Let's see how many failures we have here.
Comment #4
plachA bunch of things to fix once the testbot has completed the review.
typos
Trailing whitespaces
Nope, entity_edit() is useful as a page callback.
Fix these
Wrong indentation
Remove this one
2 days to next Drupal core point release.
Comment #6
Gábor Hojtsy#1188388: Entity translation UI in core is related, in fact this one is a sub-issue.
Comment #7
plachRerolled #4 and catched-up with entity OOP-fication.
Tests still to be fixed, let's see where failures are. FWIW the patch is performing pretty well on manual testing.
Comment #9
tim.plunkettComment #10
sunhook_forms() as well as the change to make base_form_ids multiple is unnecessary, since you have a unique and dedicated entry point with ->edit() already.
Instead of drupal_get_form(), you want to use drupal_build_form(), and supply a prepopulated $form_state to it with the base_form_id and any special args predefined. See config_get_form() in #1324618-52: Implement automated config saving for simple settings forms for an example for how to do this.
--
Lastly, I'm not happy about the term "edit" being used here. Add and edit forms are identical with regard to their entity/field API implementation and behavior, so I expect a "form", not "edit".
Comment #11
catchYeah ->form() seems better than edit() to me as well here. I only had a quick read through but I couldn't see much to complain about beyond what's already covered.
Comment #12
plachWorking on this.
Comment #13
plachAgain, tests yet to be fixed.
@sun:
I tried to use this technique but it turns out that if no function named after the form id is found
hook_forms()
is invoked, and only the values returned by the implementations are inspected for a base id. Explicitly setting the base id in the form state has no effect in this case.Moreover reverting the changes to form.inc prevents the form from being altered at three granularity levels:
hook_form_entity_form_alter()
,hook_form_ENTITY_TYPE_form_alter()
andhook_form_BUNDLE_NAME_ENTITY_TYPE_form_alter()
. IMO there could be use cases for all of them.Anyway, I changed
entity_edit()
toentity_get_form()
andEntityInterface::edit()
toEntityInterface::form()
.Do you guys think I should just fix failing tests, or should I try to go on and try and generalize a bit the actual builders and validate/submit handlers in this issue?
Comment #14
plachSorry, wrong search/replace.
Comment #16
Gábor HojtsyAdding sprint tag to make it easier to follow.
Comment #17
webflo CreditAttribution: webflo commentedPatch looks good.
Whats about this TODOs? I think this is already done in NodeFormController::submit and NodeFormController::validate
I think the patch is already abstracted enough and we just need to fix the failing tests in Poll module.
Comment #18
plachThe attached patch addressed the remaining TODOs and introduces automatic generation of form actions with the related validate/submit handlers. Moreover global submit handlers are now dismissed in favor of button-level ones: entity prebuilders are introduced to replace those and allow modules to alter the submitted values before the entity object is built from them. Tests not fixed yet, but now it should be possible to.
Comment #20
plachA feedback about the direction taken into the latest patch would be welcome before starting to fix the failing tests.
Comment #21
webchickTagging to get some eyeballs on this.
Comment #22
plachThis should reduce notices. Again, please confirm the approach is good.
Comment #24
fagoOk, here a first review. Honestly, I think it's a bit too early to start working on that monster-area, but it might be a good idea to start cleaning up this asap. So here it is:
I'm not sure we want that accessor in the entity object itself. We might want to have that, we might not as it bloats the entity with lots of accessors and contribs won't be able to add their accessors for their stuff anyway.
The primary way to make use of an an entity-related API should not be via the entity object, but via other APIs that make use of the entity object. I.e. have entity_form_controller($type)->form() as discussed in #1302378: Use multiple specialized entity controllers?
I'd agree with sun that we should talk of "entity forms", not edit form. It's for add as well as edit, so it's going to be the main entity form.
Again, I second that. There is no need for that form api changes. Again, see the entity api module implementation for an example of how you can do that: http://drupalcontrib.org/api/drupal/contributions!entity!entity.module/f...
That said, I think the current form api "entry-point" is quite a mess as we go via a generic entrance, generate specific form-ids for alterations and then map those back in hook_forms() again. We might want to clean that up first.
I must say the whole context overall confuses me and the term is already quite overloaded. Anyway, I don't think this or bundle-specific controller classes are needed. We have already a single point for form-customizations: the controller class. Override it, if necessary. I don't think we should bake in other form-customization variants - one is enough.
If necessary the controller class is going to be overridden by the entity-providing module. For modules, we still have form alters..
Why is that final?
This mixture between actions() and actionsElement() confuses me. It doesn't seem to be much difference? So is that extra-layer of specifying actions in form-api like structure necessary?
We already have http://api.drupal.org/api/drupal/includes!common.inc/function/entity_for... and entity builders. Why do we need pre-builders?
Do we handle the delete case as well? I'd see the need for X-operations then..? I think we might put some effort into operationForms instead, so you can do a "publish" operation which just presents a confirm form rather easily. See the entity api module UI controller for an example.
Why do we have static methods here?
That should be a regular entity builder.
We'll need to move all the form related helpers into the controller class. That's what they are there for. That is, we'll need to move the submit-build helper as well as the field api validation helper to the controller.
That's hard-coding "EntityFormController" although a different controller class is used. Instead, I think it's the job of the form controller to add suiting validation and submit callbacks so they get called appropriately. Then, once called there is no need for static helpers. Again, take a look at the entity api module's ui controller to get some ideas.
Should be UserAccount or UserFormController. The term "profile" is deprecated for user.module and a left-over we should not continue with.
This should define the user_account_form() itself. Then, imo the registration form should just make use of the same controller and massage the form as needed afterwards. For additional form context, we already have form state. I.e. just add something like $form_state['register'] = TRUE.
Comment #25
plachJust a quick reply to the major points, I'll have a full review tonight.
The aim of this patch is providing an initial infrastructure to ensure D8MI can proceed on the entity front, certainly not solving every problem in this area ;)
I'm trying to keep the patch size minimal, hence I didn't move the various validate/submit handler into the form controllers. I totally agree this should be done, along with other refactorings to try and reuse the more code possible. However this is out of scope for this patch IMO.
As I said I'd like the impact of this patch to be minimal, so I'd be more than happy to avoid hacking form.inc. However I tried to implement @sun's suggestion with no luck: I'll review the code above and try to understand what I'm missing.
Again, this is how things work now in form.inc: I don't know how to streamline this process and keep the ability of choosing the granularity of the form alteration (global, per-entity, per-bundle), without going the way proposed in the patches here or without deeper refactorings in the Form API.
The point of contexts (and we can certainly pick a different name, not sure which is more appropriate though) is giving the ability to provide multiple different entity forms: implementing this behavior through alterations, instead of being able to declare a different controller for each case, looks as a fragile/limiting approach to me. Also the point of per-bundle controllers is letting a module like Poll have its own specialized controller. I'd like to confine alterations to the extending modules, modules providing the entity (or the bundle) should be able to rely on a dedicated controller if they wish to.
Sure, this already somehow prepared in the code but, as I was saying above, not completely exploited yet. However I'll have a closer look to the suggested code before the next iteration. Sorry for not doing that before.
Comment #26
plachIn addition to #25, still answering to #24:
The 'consistency with contrib' argument is valid, altough I guess also having shortcuts for the most common controllers might make sense. No strong opinion here, just tell me which one you prefer.
Sure, I guess this is not particularly evident because of the messy Form API workflow, but the
entity_form()
function does exactly that,entity_get_form()
is just the Form API counterpart. However I'll try to stick more close to the naming conventions described in #1302378: Use multiple specialized entity controllers.I thought I already replaced all the occurences :)
Well, the idea behind final methods was to be able to bake-in some fixed logic and to rely on it later.
EntityFormController::form()
andEntityFormController::actions()
are just the "overridable" counterparts ofEntityFormController::build()
andEntityFormController::actionsElement()
.Having a separate call for
EntityFormController::actionsElement()
/EntityFormController::actions()
lets us provide some sane default beaviors with their validate/submit handlers already in place. In my mind most entity-specific overriding of the actions won't be needed, while certainly some customization of the form itself will. It's pure form api, btw, not form-api like stuff: it's saying just give me the actions element and (reliably) massaging it a bit afterwards.Honestly I ain't sure about those either: on one hand a piece of code needing to run before entity building even starts woud work more reliably if it could exploit pre-builders, otherwise it might be executed after the entity building phase has already begun, depending on the form alteration order. An example here would be node_field_language_form_submit. OTOH we have this kind of problem all the time, not sure whether this particular case deserves a dedicated solution.
Well, most core entity forms currently do in a way or another. This is an attempt to just keep things working. However, I will have a look to operationForms and check whether they are what we need here.
I wanted to encapsulate access to the entity object even without requiring an EFC instance, however probably making the controller more stateful and store the entity object directly into it might make more sense, right? For external callbacks we would need something like the following, anyway:
Yes, you are right. It could even be called from
PollFromController::submit()
, however (perhaps more reliably).Noted :)
I'm sorry but I don't agree with this statement: why do two conceptually different forms (aside from the fact that both deal with the user entity) need to share the same code and differentiate through a bunch of ifs when we have a cleaner way to do that which is inheritance? Why don't we just make both inherit from a base class and live two distinct lives? I bet this would be easier to maintain besides being cleaner to look at.
To sum up, my personal goal for this patch is providing the first step towards Entity Form Architectural Nirvana™. I don't expect everything to look perfect since the very first iteration. The patch is already pretty big without fixing tests, I'm afraid it'll get huge by time we manage to fix all the things we all would like to :)
Comment #27
plach@sun:
I tried once again to use
drupal_build_form()
and provide a base form id directly in the build info with no luck. The code indrupal_retrieve_form()
totally ignores it without an implementation ofhook_forms()
. I have no idea of how you made the config form work in the patch linked above :(Comment #28
plachJust rerolled the last patch for now. Trying to implement @sun's and @fago's suggestion.
Comment #30
plachI tried to make @sun's suggestion work, but touching the Form API is still needed to retain BC and make forms alterable at least at bundle and entity type level. However overall the form building process looks quite cleaner now and the Form API changes are definitely smaller.
All the rest is still to do.
Comment #32
Jose Reyero CreditAttribution: Jose Reyero commentedThough I haven't really got in dept into this patch, it seems to me that a FormController is something (an interface) we could better have in form API, then extend it for Entities.
Comment #33
plach@Jose:
I think something along those lines is being proposed in #597280: Introduce form registry to fix various problems and increase security. As I was writing above, for this patch I would be happy to just introduce a basic common denominator for entity forms. Refinements will be welcome in the follow-ups.
Comment #34
tstoecklerSeems like, because of the if/elseif/else below, the first if is never actually used. I might be missing something.
Is there reasoning the preprend $bundle but append $context? So we have "page_node_form", like we currently do? I think decreasing granularity in general makes more sense, e.g. "{$entity_type}_{$bundle}_{$context}_form"
This sounds like the whole entity type is being created. I get what it's trying to say, but I think it could be clarified.
I'm not really up-to-date on the Form API specifics. Can we have multiple "base_form_id"s? I could see both "entity_form" and "node_form" (i.e. "{$entity_type}_form") making sense. Might be a followup-issue.
This is again bikeshedding, but is there a reason your putting $submit into a dedicated variable?
Seems weird to call this 'submit' instead of 'save'. (This is the element key in the form array.)
This is a strange name. Maybe "#entity_prebuild_handlers" or "..._callbacks". Or simply "#prebuild", that would be inline with "#validate" and "#submit".
I don't understand this comment. Can you elaborate?
Here and elsewhere: Maybe we should simply document $form_state['entity'] for entity forms (or $form_state[$entity_type], though I prefer the former), and get rid of the getEntity/setEntity accessors. We really do that with all kinds of stuff in $form_state, ($form_state['values'] probably being the most prominent example), so I really can't come up with a valid reason against this.
Seems like the other entities are missing this change. Is there a reason for that? I mean tests pass, so I guess there is...
Seems like maybe we could just be nice in the parent submit function and pass $entity directly to the save function?!
I might be wrong (I didn't try this patch out), but doesn't put a "Delete" button on the user registration form? That would be strange...
On the final methods: I think splitting actions() and actionsElement() makes sense for the reason you noted, but I don't think we need to make actionsElement() (and all of the other functions) final. We should document, that in the 90%-case all you have to do is override actions() (or not even that, but that's not the point), but if you want to go crazy, you can override actionsElement() and do whatever you want.
Regardless of whether we have a generic FormController or FormControllerInterface we should have an EntityFormControllerInterface that EntityFormController implements.
Regarding $context: I think $variant would make more sense. I also contemplated $form_view_mode (or just $view_mode?). I think we should avoid $context, as it's already has roundabout 20 meaning within Drupal.
Comment #35
plach@tstoeckler:
Thanks for the review (I saw it right now), you are raising some valid points! I'll try to address them along with fago's ones after fixing the tests (OpenID ones should be ok now).
Bot?
Comment #36
plachwtf?
Comment #38
plachThis should fix some more tests/bugs.
Comment #39
plachGreat! Now that we are sure that the current approach is not irremediably broken I'll try to address the pending issues from the reviews above.
Comment #40
plachI started implementing @fago's suggestions. The attached patch removes most static methods from the EFC and improves entity encapsulation. Posting it just to see if something broke, but it's only intermediate work.
Comment #42
plachThis should work a bit better :)
Comment #44
plachAgain, this should work better. Working on the rest of the reviews.
Comment #46
plachRerolled
Comment #48
tstoecklerI read through this again, and am really liking this a lot. I found a bunch of more things to complain about, this time I went really nitpicky with docs and such, though. So nothing major.
Let's break that into multiple lines.
Can we add a @todo to EntityFormController::actions about adding a 'Preview' action, since a bunch of entities in core already duplicate that?
I don't think that needs to be done in this issue, but I think we should add @todo's to remove the procedural functions alltogether.
There were a couple and typos and things I found weird in this paragraph, so I took the liberty of directly providing an improved version, instead of a bunch of detailed, but minor reviews. It's (quite) a bit more verbose, but hey...:
I'm not quite sure where, but I think the word "entity" should be in there somewhere. :-)
I think the "parts of it" part can be omitted. I'm not sure what you mean by it, but it doesn't seem essential in this function header.
Since this basically describes, what is documented above in hook_entity_info(), maybe add a @see?
In the second sentence the "valid" can be ommitted I think (since we really perform no validation).
This key is not documented in hook_entity_info(). Also, shouldn't it be inside the 'bundles' array? I.e. rather $entity_info['bundles'][$bundle]['form controller class'][$context]? If I'm not mistaken, that would even allow a bit more flexibility in terms of bundle-specific overrides.
Omit the "Helper function." part.
Now it's getting really nitpicky, but maybe replace "the" with "a"?!
I generally really dislike these sorts of double variables. In theory, we could call this "entity_type_get_form($entity_type, $bundle, $context)" and then have a separate "entity_get_form($entity, $context)" call this with $entity->entityType() and $entity->bundle(). The way it is now is kind of backwards IMO, but on the other hand I'm not sure whether two functions is actually worth it.
I always omit that blank line, is there a standard for this? (I'm genuinely asking, it might very well be that this is the correct way.)
Seems like this could be more easily achieved by simply adding a default weight of 5 or something to the 'Delete' action. That would collide with the setting of the weights below, but I don't really get, why we do that anyway.
double space between "submitted" and "entity"
Maybe simply "Executes entity prebuilder callbacks." (Or whatever the proper name for these things is.)
Is there some sort of standard for this? With foo_bar_form_submit() we do "Form submission handler for foo_bar_form()."
Copy-paste please! :-)
Hmm... If we want people to be able to subclass this easily, shouldn't we use $this->getEntity() intenally as well. Otherwise if people override getEntity(), the above won't work.
IMO it's super weird to have one of the static and the other not. for setFormInstance() we could simply pass $this explicitly when calling.
ids -> IDs
comma after "provided"
Should now be "Implements hook_form_FORM_ID_alter()."
for -> form
Also neither of these are overriden or subclassed anywhere, so I don't think it's reasonable to call them "base form controllers"
For taxonomy_term you renamed taxonomy_form_term() to taxonomy_term_form() (which totally makes sense). Don't know if we want to do likewise, here.
I know this wasn't introduced here, but can we please add a @todo to not call taxonomy_vocabulary_confirm_delete() from within taxonomy_form_vocabulary() ?
Comment #49
tim.plunkettI can't find it at the moment, but a blank line after the class declaration is correct:
Comment #50
plachThe attached patch implements the suggestions from #24, #34, and #48 with some exceptions. In many cases the reason behind not implementing a particular suggestion is keeping the patch size minimal. I'd wish we could commit something similar to what we have here and agree on some follow-ups to clean things up in smaller chunks.
It's too late now to provide a detailed description and rationale. I'll do it tomorrow along with a list of the proposed follow-ups.
Meanwhile let's see if the bot is happy. Attached you can find also an interdiff with #46.
Comment #51
plachHere are some detailed answers:
@fago:
Removed
EntityInterface::form()
and leftentity_get_form()
as the main way to retrieve an entity form.As suggested by @sun in IRC and in his sandbox, the attached patch uses
drupal_build_form()
and removes the implementation ofhook_forms()
with some minimal tweak to the Form API. The main reason behind those is retaining BC and not being forced to rename all thehook_form_FORM_ID_alter()
andhook_form_BASE_id_alter()
implementations all over the place. If also this solution is deemed too invasive I'd try to move the call todrupal_build_form()
intoEntityFormController::build()
and return a renderable (form) array.The current approach retains the ability to alter the form at bundle level and entity-type level. We cannot alter any entity form anymore, but this behavior can be "emulated" by implementing
hook_form_alter()
and checking thatEntityFormController::getFormInstance()
returns a valid form controller instance (a non-false value).After studying the Entity API's UI controller more carefully, I noticed some similarity between the concept of context here and the operation forms there. For this reason I didn't try to remove it for now, as I'd like to discuss/explore this similarity some more. However per-bundle controllers are gone, I'd open a dedicated follow-up for them if anyone here thinks they shouldn't be just dropped.
I provided an answer to this point in #26. Any feedback?
Prebuilders are gone now.
Statics are gone. The entity object is now stored directly into the controller as a protected field. This improves encapsulation and should give us a more reliable way to access the entity from form the various callbacks, instead of the bunch of inconsistent ways we have now.
This now happens only for the form controller instance static accessors which are final as there should be no need to override them. Better enforce some consistency here. They are needed in any "external" alter/process/validate/submit callback to access the entity.
I already replied to this statement in #26. Quoting myself:
What do you think?
@tstoeckler:
The main reason here is keeping BC, for instance having
user_profile_form
, but your proposal makes sense. We might want to address it in a follow-up.Well, this was exactly the approach I originally chose and that both @sun and @fago refused as it implied some changes to the Form API.
Just readability, the array looked less clear to me.
Again BC: we would need to change all the places accessing 'submit' key. A follow-up would be appropriate here.
Not in my mind: D8 is moving to OOP, which provides encapsulation, which in turn provides more reliability/testability/ease in refactoring things and all this stuff you know better than me. Ax a general rulem, I think we should exploit this capabilities whenever they make sense and keep structured arrays mostly for dynamic data definitions, such forms and info arrays.
No :) Look at
EntityFormController::actionsElement()
.Not sure about this: we can still rely on alterations to change anything on the form we wish to. IMO it's important that we somehow codify what is supposed to be changed and what might be dangerous to. A final method is a clear indication, a PHP doc might be ignored or missed.
This depends on whether we want to make the entity form controller implementation swappable: I don't see many use cases requiring to entirely replace such a complex system. This might use a bit more discussion.
Why? In most of the places I saw it's just one line.
Me too :) The string-only usage is gone.
Again it's mostly to retain pseudo-BC: it seems a common pattern to use a "5 step" weight among action widgets.
I'd tend to agree, put we should make the
$entity
field private, then. It seems the general trend for DrOOPal is using protected variables, so I picked this visibility mainly for consistency. Any other thoughts about this?I tried but some tests broke and fixing them implied unnecessary changes that would increase the patch size.
Aside from the ones hinted in the answers above, here is a list of follow-ups I'd open after committing (something like) the attached patch:
What do you think?
Comment #53
plach#51: entity_forms-1499596-51.patch queued for re-testing.
Comment #54
chx CreditAttribution: chx commentedplach asked me to look at the form.inc change, it's a good one we should do more of that in #1454730: Allow OO methods as FAPI / render API #callbacks (which smells major task to me now).
Comment #55
plachA small improvement: the attached patch makes the entity form state reusable and slightly improves the entity form API.
Reviews welcome :)
Comment #56
fagoThis actually doesn't show a comment edit form, does it? It's a comment add form.
Just comment form - no edit?
I know you try to avoid moving code around, but implementing a class for just forwarding the call is kind of strange. Still, we could clean that up in follow-ups.
Instead of keeping entity-type specific submit_build_entity functions, I think that's something that needs to go into the controller. Then, the controller can make use of it for invoking submit handlers that already get the entity passed.
Actually, that's wrong. It's no submit handler any more and that is where just forwarding calls to existing submit handler starts getting really confusing.
Also, that code does away with the submit builder? (Again, if the builder would run on the controller level and this would just get the entity passed would simplify things).
This patch introduces a submit-phase for entity forms, but that duplicates the already existing submit-builders. That's supposed to be the same thing, e.g. see node_form_submit_build_node() which invokes node_submit().
Again, edit terminology.
@validation/submit handlers:
I could not see where those a registered for the main form, so I guess entity-forms would just fallback to form_id_VALIDATE callbacks? That would be very problematic if the control flow for invoking validate/submit handlers differs depending on whether a button adds the controller validate handler or not. The controller valdiate should be the default and the only way to invoke entity-specific form validation.
@entity-controller & instances:
I don't think it's a good idea to introduce that state to the controller, first off it doesn't fit to the concept of controllers so it would make it something else. I think it would be better to keep the entity in $form_state only + add the controller instance in use as well. Then, access to the entity can still go via the controller, ala $form_state['controller']->getEntity().
For submit-building I think we want to have a way to build() and get the entity, ala $form_state['controller']->buildEntity() - what builds the entity based upon submitted values. Then there should be $form_state['controller']->submit() what builds the entity + updates the entity in form state. That should be default submit handler and works fine with form-rebuilds, while buildEntity() can be used to get the updated entity object for validation or such (to be figured out in follow-ups).
+1 for supporting OO #callbacks
I agree that it makes sense to leverage inheritance for build and controlling that form. However, still I'm not convinced we need to invent that $context system as it's a system for explicitly supporting multiple variations of entity forms.
Imo, its' important that we have a clear and single way to get and adapt the entity form. But that any variations of that form need not be registered in the system, that should be possible by implementing my own form + use the entity form controller + add in my customizations or I use my own version of the form controller to add in my customizations, i.e. I extend the controller, instantiate it myself and use it to control the form.
That said, form alterations of entity forms should happen independently from the real form-id of the form, what could be seen as a problem of general form re-usability and deserves its own issue as well.
Thus, imo registering multiple variations of entity forms should be up to a general form registry, as I don't see the need to re-use multiple variants of entity form specifically.
Note that operations forms (delete, clone, export, ..) are not variants of the main form, they are actually totally different forms. So we should have different mechanism for them, although it might make sense to leverage the same controller? But again, that can be a follow-up.
Comment #57
Gábor HojtsyEssential for translatable content.
Comment #58
plachI know, but I'm pretty much sure you'd have more chances to be confused with a 200k patch :)
IMHO having one issue for each core entity to clean up those things and factor up common code is going to be easier to achieve than doing all the hard work here. However by proxying those calls we are ensuring that the cleaned-up code is likely to work without problems.
I ain't sure I understand your concerns here, so I'm summarizing the current behavior to ensure we have a common ground for discussion:
This is actually splitting the submit-build phase, which may be needed by every action, and the actual submission logic, which is confined in the 'save' submission handler. The base 'submit' handler is not duplicating the submit-build phase, it's just moving it into the controller, which is what you seem to suggest above. This is why I'm a bit confused.
Moreover I ain't sure I get what's the problem below:
Currently the validation handler is added by the controller in the overridable
EntityFormController::actions()
method. Are you saying that it should be treated as the submit handler and always added to the 'save' button in the following non-overridable action massaging? If so I'd tend to agree.Agreed. I grudgingly removed the entity encapsulation as your argument above about consistency between controllers makes totally sense. I'm glad you're fine with keeping the
getEntity()
method as I really want to introduce a unique and consistent way to access the entity object. To do that I had to add the form state as a wrapped field, but I guess this makes more sense and does not violate the definition of controller.The attached patch introduces a reusable
buildEntity()
method that for now is invoked only in the submit-build phase, but that could be theorically used also in validation handlers. Freshness is ensured by a process callback that updates the wrapped form state after cache retrieval and before handlers are invoked.Let me explain my point a little more: after looking better at the current Entity API code, I started thinking that what we are likely to need are not micro-variations of the default entity form, but actually a different form for each operation to be performed on the entity: add/edit (default action), clone, export, delete and so on...
What I'm proposing is keeping the context system here and renaming it to use a "operation" terminology so that we can have a common logic to register and instantiate a form controller for each needed action. Currently only the user entity type need contexts to be implemented cleanly (i.e. with a specific form controller for profile and registration forms). This fits perfectly the scenario proposed above, since we could see it as an exceptional case where the 'add' and the 'edit' forms are (slightly) different.
Comment #59
plachComment #61
plachRerolled. Interdiff same as in #58.
Comment #62
fagoI see - so that moving it into the controller without actually removing it is what actually makes this totally confusing and confused me. So this patch duplicates a lot, without removing existing stuff, while partially using the existing stuff. But then partly, existing stuff is used without incorporating the added stuff, what creates quite a mess and makes the whole already difficult to follow code massively more complex and the whole situation worse.
Instead of that, we need to clean things up and create a central point of control: the controller. Thus, no old submit handler may be called directly any more, best they are incorporated into the controller. Then there should be no back-and-forth between controller and non controller functions, e.g. buildEntiy() vs node_form_submit_build_node() vs node_form_submit() and controller save() and submit(). That flow between those functions is just confusing me. node_form_submit(), node_form_submit_build_node(), node_form_validate() - those all are deprecated by that patch, instead only the node's controller should be invoked.
Thus, I really do think that we need to incorporate moving old handlers to the controller to clean up things here. Sry, I know you've been trying to avoid that, but I don't see that working out otherwise. Also I don't mind reviewing a 200kB patch, in particular if we've Git, revisions and interdiffs. But if you prefer to keep things small, I'd suggest splitting the conversion by entity type instead.
If submit-building the entity is activated for a button, validation needs to be activated as well. For deletion, both can/should be passed.
I say that all validation/submit handlers clearly need to point to the controller, not in some cases bypass it.
yep, exactly. I'd call the add/edit form the main entity form, others are special operation forms and handled differently.
export/clone/delete operation form have mostly nothing in common with the main entity form, so I don't think using the same form controller for that makes a lot of sense. Maybe, we could do a small operation-form-controller that in particularly eases customizing text messages? Not sure, but anyway I'd prefer to leave that out of this issue.
Regarding contexts, it might make sense to introduce that as something like "edit_mode" - analogously to view_modes. So "register" and "add" could be different edit-modes, maybe? Anyway, again I'd prefer keeping that out of this issue as well.
Comment #63
plachOk, I'll start moving the various extrernal functions into the controllers. Not sure about factoring code up into the base class: should we postpone that to a follow-up?
Currently the submit-build phase does not implement any "business" logic, it's just an "extension" of the Form API flow. The actual logic is into the action-specific submit handlers. I'm not sure what to put into a validator that would make sense for all the actions. However I realize that the same validation logic may apply for instance to save and preview actions. Should we have various validate methods, one for each action, like the submit ones?
Agreed, If there is a validator it totally needs to be on the controller. I was just saying that some actions don't seem to require a validation handler.
I'm not saying that the various operations should have a shared controlloer, I'm just saying that from the DX perspective it would be nice to be able to do something like:
Then I might have some operations having a common parent controller, for instance both the
AccountFormController
(user edit action) and theRegisterFormController
(user add action) could inherit from a genericDefaultFormController
, but this would not be required at all. There would be only a common "entry point".Yeah, this somehow reminds view modes, as also @tstoeckler was pointing out. Totally agreed on not addressing it here.
Comment #64
fagoGreat! If we convert all entity types at once, I'd do so - maybe we can simplify them a bit as the controller has knowledge about the entity location. But as you prefer - keeping them shouldn't complicate the overall flow much.
Submit-building extracts the form values and gives you an updated $entity. If you are using this entity during #submit, you need to add the regular entity #validate as well, or your $entity contains unvalidated values. Only dealing with validated values is the whole idea of the #submit phase :) - if you only partly validate the form you should only access validated form values (that's way non-validated form values are missing from $form_state['values']).
Comment #65
plachWork in progress patch for the bot.
Comment #67
plachHere is the big one. Validation handlers fixed as per #62. Contexts/operations still there. Let's see what the bot thinks abut this one.
Comment #69
plach#67: entity_forms-1499596-68.patch queued for re-testing.
Tests passing locally.
Comment #70
plach[text moved to #75 below to improve readability]
Comment #72
plachFixed a small issue in the Translation module.
Comment #73
tim.plunkettComment #75
plachThe attached patch removes all the occurences of
$form['#node']
and friends. Not sure how to proceed with contexts as many things break for users since we cannot have two distinct form ids anymore without them. For now I just changed the terminology from context to operation.This looks good to go to me. Fago?
Comment #76
plachUnintended change.
Comment #77
plachRerolled
Comment #78
plachComment #80
plach#75 was missing.
Please guys let's finalize this and get it in: it's getting awful to reroll :(
Comment #81
BerdirGreat stuff, really like this. Looking forward to being able to unify those forms (e.g. save callbacks) to actually save code in the end instead of adding 400 additional lines ;)
Review below, mix of minor things that should be cleaned up (setting to needs work for that) and some notes/questions.
Note that you're missing one entity form in field_test.module, but that has not been converted to an actual classed entity yet, so doesn't make sense yet. Conversion is happening in #1374030: Remove entity_extract_ids() now that we have proper classed entity objects. Note that there's some crazy stuff there like a form with two parallel entities that we probably can't deal with in a generic way.
Did the review of the code in #77, FYI.
What is exactly the reason for using that static method here instead relying on $form_state['controller'] directly?
It's neither a protection against $form_state['controller'] not existing (it would return FALSE in that case and would then explode) nor does it allow to inject/mock anything, it's a hardcoded static function.
$form_state['controller']->getEntity() would IMHO be easier to read.
If there is a reason and I'm not seeing it, care to explain? :)
While a valid change, does look a bit unrelated, or are there any side effects of this which aren't visible here?
I think the standard docblock for this is "Overrides ....".
Note that entity_form_field_validate currently casts $form_state['values'] to an object and uses that as $entity, we need to find a proper fix for that, my entity_extract_ids() issue currently changes that to an entity_create(), which is imho also wrong.
Is this still used in core?
I guess we need a list of follow-up issues to have an overview of the remaining tasks, there are lots of important @todo's in here. Having that list and having a meta-issue or a tag that shows that we have a defined path towards additional improvements could help to get a patch with that many @todo's in. Having a major meta issue with issues for each @todo would show that it's a temporary thing.
I guess that's what should also do for validate(), maybe we could actually have maintain two entity objects, one with the existing values and one that is updated with the current form values? Something for later, of course.
@param $form_state is wrong. $entity should be EntityInterface
Powered by Dreditor.
Comment #82
plachFYI, I created a new sandbox to work on the Entity Translation stuff: http://drupal.org/sandbox/plach/1719670.
Comment #83
plach@Berdir:
Thanks for the review!
Yeah, those 400 lines annoy me too, but we should consider that we already have tons of lines that are no longer loaded in the various .pages.inc files :)
Well, the main reason is having an API in place: it's true that it does not provide protection/encapsulation or anything, but this way we are exchanging a raw data structure, which becomes an injected dependency (taa-daa), with a true API. People can still use the form state
'controller'
key but they do so at their own risk: we might change that key or refactor the getter logic in any moment of the D8 lifecycle and they would not be allowed to complain :)However if some of the crazy ideas I read in an issue linked somewhere above come true, we might end up with controllers for any form, and we might even get an OOP-fied form state, which would allow us to write something as beautiful as
$form_state->getController()
:)Well, deletion confirmation is an entity form, just a different operation. I thought it might make sense to fix also that one here. Do you think it would be better to leave it out?
Yes, the idea is exactly to use
buildEntity()
also for validation. However we need to moveentity_form_field_validate()
in the controller or at least refactor it to get an entity instead of using the form state.Yes, I don't remember exactly where, but tests fail without it.
Totally agreed. As soon as this goes RTBC I'll prepare a list of follow ups.
Comment #84
BerdirChanges look good, just one tiny thing...
Here was @see actually correct as it's not a hook-like single-line docblock but a reference after that.
Comment #85
plachSorry for missing that.
Comment #86
Gábor HojtsyShould still pass with that phpdoc change. Otherwise passed very rigorous reviewers, so let's get this going!
Comment #87
fagowow, great progress!
Still, as this goes rather deep before going in I think it needs another architectural sign-off from someone involved into the whole entity-form mess, e.g. from sun or effulgentisa!
So let's start with some architectural review first:
First, I must say the code is much clearer and easier to follow now! I like centralizing all the stuff that belongs to a form in a single class, that makes it much easier to see what's there.
We need an interface, e.g EntityFormControllerInterface so I can roll my own and people know which methods they can rely on! Then this class should be just a base-form controller I may use.
I don't see the need for this static-getFormInstance either. The static method cannot know where the controller is either if an implementation customizes its location.
Therefor, I think we should just go with calling $form_state['controller'] as well.
This has confused me a bit when going through the code. When looking at preview I was missing the entity-building and update. Furthermore not every button needs this update, it's actually important that the button decides whether to update data or not *just as* it decides whether to validate data or not. Thus, I think adding the validation and the submit callback should not be automated like that but done manually for each button. That way it's much easier to see what's going to happen when it's pressed + we do not update the entity or validate for an delete button.
ok, then here a regular review of the code:
That's not true, isn't it? I think it's still $form_state[$entity_type]
Then, if I do an entity form with 2 or more entities, it might be desired to work with multiple controller. That wouldn't be doable with this static call.
Yes, this $form['#node'] entity locations are already deprecated for D7. It should be one of the follow-ups to remove all entities sitting in $form.
This needs better docs, what are those operations???
Docblock misses data types. Others in the controller class too.
I don't think we should have final methods? What would happen if I decide to customize that? Why forbid it? Same for other final methods?
hm, that looks strange. Why is that needed? Updating the entity object in the controller instance should suffice?
$entity isn't used here?
Good. Can we incorporate the great docs from http://api.drupal.org/api/drupal/includes!common.inc/function/entity_for... here? Or why is that the default-bulder if buildEntity() does the building? Maybe it should be just clarified what'S the difference to buildEntity(), i.e. it updates the already validated entity in form-state for future rebuilds or saving ?
Let's do so?
Does that mean preparing the entity for editing? Then it should say so.
It should be documented at buildEntity that it clones, then it should be natural to go with it here.
I'm still not convinced that this belongs into the same controller. In addition as it doesn't seem to be used e.g. for node deletion.
Shouldn't that button just have it's own submit handler? But I see that's copied over code, so cleaning it up doesn't belong in here.
Comment #87.0
plachAdded first stab of an issue summary.
Comment #88
plachMost of the suggestions in #87 are implemented. Some answers:
Nope, see
EntityFormController::getEntity()
.Would you clarify this? Maybe it was referred to the controller static accessors?
Yes, I left out occurrences not belonging to entity forms to avoid bloating too much the patch.
Yes, it's a bit weird but if we don't update also the reference after the form is retrieved from cache, handlers not belonging to the controller will act on a different controller object and thus won't get the updated entity.
I can't locate that line, but all the occurences of
$entity = $this->getEntity()
I could spot were using the returned entity.Well,
entity_form_submit_build_entity()
is used also in field tests, it would be easier to address this in a follow up.Almost all converted entity forms have a delete submit handler which redirects to the deletion confirmation form. I think it makes sense to have that one since it is more than likely that we'll end up with a unique shared implementation. Instead I think it would be confusing to have only that one implemented as a regular, procedural submit handler.
NodeFormController::delete()
redirects to the deletion confirmation form and so do the term form controller (also the vocabulary one should). The Profile form controller has a procedural cancel handler that would also fit into this pattern IMHO.Comment #90
fagoThanks, for the update and the answers. Changes look good.
Oh, yes. That's problematic, I guess we should make sure there is only one cached controller? E.g. we might want to combine form-state and form in a cache. Maybe let's just an todo there so we don't forget and create a follow-up.
This seems to be only the place left who is hard-coding the location of the controller in $form_state inside the controller. We should avoid that for enabling multiple-entity forms in a single form (what quite some modules do).
Oh, I see. Removing $form_state['node'] is an API change with a rather high impact. As we are moving more and more to entity-generic code I think it makes sense. It should be good visible in the change-notice though.
Comment #91
plach@fago:
Do you mean that the
'controller'
key should be variable per form/form state?Comment #92
plachSorry, wrong patch. The interdiff in #91 is still valid.
Comment #92.0
plachUpdated issue summary.
Comment #93
plachDone
Comment #94
plachJust realized that a callback altering entity forms in a generic fashion might need the operation in addition to the controller to know which one it is acting on.
Comment #94.0
plachUpdated issue summary
Comment #94.1
plachUpdated issue summary
Comment #95
plach@fago is working on some cleanup
Comment #96
fagook, as said I've worked a bit over this:
I've not touched the new entity.module helpers much, in particular the $operation related handling. I'm no fan of the $operation term, while I think introducing the concept makes sense. It's the analogy the view-modes for entity-forms and if field API would support it, it would help a lot to ease customizing registration forms or configuring different form-widgets for searching? I think this needs some more though and probably a rename as it doesn't map to operation (that would be e.g 'delete', 'create'?). Anyway, given that this blocks some further important work I guess we can care about that in a follow-up.
Comment #97
fagoComment #98
plachChanges look good to me, thanks. Obviously I'm not the right person to RTBC this. We may want to wait for @effulgentsia to have a look to the issue.
Comment #99
BerdirCan we switch to entity_create() here for the time being instead of the object cast so that we have a classed object? I had to do that to get the entity_extract_ids() stuff working in #1374030: Remove entity_extract_ids() now that we have proper classed entity objects.
Comment #100
plachThe attached patch fixes #99 by using
buildEntity()
as suggested by the @todo.Comment #101
fgmMight be a good occasion to include "form_modes", the form equivalent of "view modes", as introduced in the entityforms module (not the one in drupal.org/project/entityform, another one I can't get my hands on again, probably a DamZ' sandbox).
Comment #102
plach@fgm:
Yes, this has been proposed a couple of times during the discussion. It would certainly be an interesting follow up.
Comment #103
fago@#100: Looks good!
@fgm: This patch basically does, see my notes about $operation in the last paragraph of #96.
Comment #104
plachRerolled after
entity_extract_ids()
has gone.Comment #106
plachFixed tests. Below a list of proposed follow-ups, if we agree on them, I'll start creating the issues.
form_state_values_clean()
intoEntityFormController::validate()
entity_form_submit_build_entity()
intoEntityFormController::buildEntity()
TestEntityFormController
for field test [maybe: use a single test entity for tests so we have to do this only once]The nested bullets may or may not deserve a dedicated issue.
Comment #107
effulgentsia CreditAttribution: effulgentsia commentedI haven't reviewed this in detail, but architecturally, I'm very +1 for this. I agree with the form.inc changes and with EntityFormControllerInterface. Even if there's flaws in the patch (and with something this big, there likely are), I'd be ok with letting them get found and dealt with in follow ups. If I manage to get enough time to review this in detail before DrupalCon Munich, I will, but please don't hold this up on me if it manages to get RTBC'd before then, since it would help unblock other D8MI entity issues to be worked on during the conference sprints. Thank you, plach, fago, and everyone else who's helped with this awesome refactor.
Comment #107.0
plachImproved text
Comment #108
fagoThanks effulgentsia! Let's move on then and complete the list of follow-ups and API changes for the summary:
Here some more points for follow-up tasks:
Then, for the API changes I miss the important point that $form_state['node'] does not longer work.
Comment #109
plach@fago:
Thanks! I'll start creating the issues later today, it seems there's some overlap between our lists btw. In a couple of cases I'll just create a stub and I'd ask you if you could complete the OP, because I am not totally clear on how to proceed.
Comment #110
BerdirAnother follow-up: Move the test_entity form handling in field_test.entity.inc to the form controller. I hope that it should become a nice example on how the form controller works as there should be no/few special cases, unlike our real entities. We could do that in #1616930: Fix the field_test entity type, not sure.
Comment #111
plach@Berdir:
I meant exactly this in #106:
Sorry, if it was unclear. IMO we should also unify the test entities: now we have one for entity tests and one for field tests. I agree we could use the issue above.
Comment #111.0
plachAdded a list of proposed follow-ups.
Comment #112
plachUpdated the issue summary.
Comment #112.0
plachUpdated issue summary.
Comment #113
sunThis looks awesome!
I have a couple of remarks, but I'd really like to see this patch to land ASAP.
So all of the following can be discussed and handled in one or more follow-ups:
(Also note that I did not read the issue nor previous comments yet, as I wanted to review this from a "fresh" perspective. I'm sorry if I'm duplicating/repeating earlier discussions/comments.)
These use statements are added to many files, but the actual/current code calls into an injected $form_state['controller'] now, so they're no longer necessary. I suggest to clean them up after commit.
Delete confirmation forms are not converted in this patch yet? In any case, let's make sure that we're consistently using $form_state['entity'] everywhere. That can be happily cleaned up in a follow-up, too, though.
Shouldn't this pass the full $node object sorta as an additional "context" into entity_get_form()? That's a more complex case, which we can happily discuss in a follow-up.
It looks a bit odd that we're calling into the base implementation at the end instead of at the beginning. However, we can discuss that in a follow-up.
It's not quite clear to me why these functions do not live on the base EntityFormController, but I'm happy to discuss that in a follow-up.
Some of the EntityFormController's method names are continuing the bad & ugly method naming practice that we began in the Entity class. I'm really not happy about some of these method names, but I'm fine with fixing their names in a follow-up.
I think it would make sense to store a reference to $form_state in a protected $this->formState property within the EntityFormController to avoid having to pass the $form_state argument all over the place. However, we can happily do that in a follow-up.
Comment #114
plach@sun:
Yeah, those are leftovers of when we had static methods to access the controller in the form state. Agreed on rolling a quick follow up patch after commt.
Not yet, I felt that 200K was a sensible limit for the patch size :)
Sure, I guess every entity form should rely on the same internal key in the form state, unless there's a good reason to make an exception. I'll add a follow up to the list for this.
Do you mean using a dynamic argument list in
entity_get_form()
to pass further parameters to the controller? That might make sense.Not sure why, I've always seen such things in OOP code. However we can definitely address this in the issue merging/factoring-up the various
form()
methods.Those are at the same level of
entity_get_form()
(actually it depends on most of them), hence we cannot move them into the controller without introducing some weird intantiation pattern or making them static methods, which have been banned from the base controller implementation.We totally need to address this (somewhere else): I saw @Dries complaining about entity method names in two or three different issues already :)
For now I just tried to stay consistent.
This was the way we did it until #96, where we discovered this approach has problems. There are a couple of related follow ups planned.
Comment #115
Crell CreditAttribution: Crell commentedThat's completely legit. You can call parent::whatever() at the start, end, or even middle of an overriding method depending on what's appropriate for your use case. Beginning is just the most common. (I haven't followed the code here enough to have any sense of whether that's appropriate here or not, but there's nothing inherently wrong with it.)
Comment #115.0
Crell CreditAttribution: Crell commentedUpdated issue summary.
Comment #115.1
plachAdded follow up issues.
Comment #116
plachMerged the suggested follow ups from #106, #108, #110 and #113 in the issue summary.
Creating the issues.
Comment #116.0
plachUpdated issue summary.
Comment #116.1
plachUpdated issue summary.
Comment #117
plachSummary updated with the new issues. I marked the following "To be completed", as @fago can provide a better overview:
#1728788: Introduce hooks for altering, entity-building, validating and submitting an entity form
#1728812: Prevent entity form controllers from being cached twice (in $form and $form_state)
#1728816: Ensure multiple entity forms can be embedded into one form
#1728818: Clarify $operation and the relationship to form modes (as a counterpart of view modes)
Comment #118
plachtagging
Comment #118.0
plachUpdated follow-up list.
Comment #119
Dries CreditAttribution: Dries commentedI looked at this and it looks promising. I do want to spend a bit more time on this though, before committing it.
Comment #120
catchI started reviewing this but lost my dreditor session. Only had minor complaints so if Dries beats me to finishing his review I'd not want to hold it up.
Comment #121
sunWould be lovely to have this in core before the weekend, since we're having an Entity API + Field API + Config system + D8MI sprint.
Comment #122
catchWe could type hint $node, and there's no docs for the function parameters.
I think this exception was added for entities with no bundles etc., not for entity types themselves. We should probably have two different exception classes for those?
British spelling mwahahaha.
The only thing that really sticks out to me is the submit() vs. save() methods. We usually call $entity->save() in a form submit handler, but now we do that in the save() method (assuming it's overriding the default class since it's not in the interface at all). I think the default class could use more docs on the save() method to indicate what it's for.
None of this is serious to stop me committing it so I'll do that later tonight or some time early tomorrow.
Comment #123
plachRerolled the patch after the latest commits and took care of the clean ups mentioned in #113 and #122, plus a couple I found myself meanwhile. The actual code is untouched so keeping the RTBC status.
I switched to InvalidArgumentException, which seems appropriate to me. We may want to introduce more meaningful exceptions in another issue.
Comment #125
plachForgot a backslash :(
Comment #126
aspilicious CreditAttribution: aspilicious commentedbackslash isn't needed, I think...
Comment #127
Gábor HojtsyShould be back to RTBC. We can remove that backslash later if that is better.
Comment #128
catchAlright. This is blocking a fair bit of work, and looks in very good shape. Thanks for fixing some of the follow-ups prior to commit.
I've committed/pushed to 8.x now, leaving this open for the change notification etc.
If there's follow-up issues found for bugs or clean-up for this, it'd be good to open new issues for those but link to them from this one.
Comment #129
plachCool, thanks!
Here are a couple of change records:
http://drupal.org/node/1734540
http://drupal.org/node/1734556
Comment #130
plach@catch:
Agreed. Maybe we can roll a quick follow-up if @Dries has any pending concern that does not strictly require a dedicated issue.
Comment #131
vasi1186 CreditAttribution: vasi1186 commentedI've read the change notices, they look fine for me, I easily understood them.
Comment #132
tim.plunkettI think the change notices are still incomplete, it took me more than a couple minutes to figure out where user_profile_form() went.
Maybe a list of the entity form functions that were removed? So someone googling "where did user_profile_form go in D8" can find the info?
It should also have an example of using this in a hook_menu, I think.
Comment #133
fagoComment #134
jvns CreditAttribution: jvns commentedI've updated the change notices to be more clear, including adding a list of entity form functions that were removed. Still no hook_menu example, though.
Comment #135
plachLooks good to me.
Tim?
Comment #136
jvns CreditAttribution: jvns commentedAdded a hook_menu() example
Comment #137
jvns CreditAttribution: jvns commentedoops, fixing issue status
Comment #138
tim.plunkettAwesome! Thanks.
Comment #139
Gábor HojtsyThanks everyone! Amazing!
Comment #139.0
Gábor HojtsyUpdated issue summary.
Comment #140
Gábor HojtsyShould be off the sprint, thanks all!
Comment #141
Gábor HojtsyBonus: this is getting a lot more test coverage in #1757232: Improve test coverage for the entity form controller once committed :)
Comment #143
chx CreditAttribution: chx commentedComment #144
chx CreditAttribution: chx commentedtry #2
Comment #144.0
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #145
donquixote CreditAttribution: donquixote commentedI found this issue after some
git blame
.In
form_builder()
at the time of the patch, but nowadays inFormBuilder::doBuildForm()
:Is there a reason why this uses
call_user_func_array()
instead ofcall_user_func()
?I think the latter would make the by-reference stuff easier, right?
In general,
call_user_func()
is good for a known number of args, whereascall_user_func_array()
is good for an unknown number of args. I would expectcall_user_func()
to be faster because it is less dynamic and does not make you deal with arrays. But I have no evidence for this atm.Why am I digging this up?
- In #2096731: form_builder() pointlessly passes $element by reference to #process callbacks it was questioned why we pass the $element by reference, when we are really interested in the return value.
- In #784874-13: Add ability to insert FAPI wrapper element without modifying #parents of children I identified a misbehaving
#process
callback,FormTestFormStateValuesCleanForm::cleanValue()
.These obversations led me to a closer look at this mechanic.
Maybe participants of this issue still remember why
call_user_func_array()
was used.Comment #146
donquixote CreditAttribution: donquixote commentedSorry for changing the version. I think this happened automatically because 8.0.x was no longer available. Maybe a release tag should be chosen instead?
Comment #147
donquixote CreditAttribution: donquixote commented8.0-alpha2 is the first/oldest available tag in the select list for the 8.0.x branch. The patch (commit ea25df8) was applied before 8.0-alpha1, but this is not in the list.
Comment #148
donquixote CreditAttribution: donquixote commentedSorry for the noise.
The call_user_func_array() is required because call_user_func() does not pass parameters by reference.
http://stackoverflow.com/questions/13798468/how-can-i-pass-reference-to-...
If a function expects by-reference parameters, then call_user_func() will result in
"Warning: Parameter 1 to foo() expected to be a reference, value given in .. on line .."
https://3v4l.org/5ojUk
I could have figured this out by myself before posting.
Comment #149
donquixote CreditAttribution: donquixote commentedBut we could have stayed with the
$process($element, $form_state, $form_state['complete form'])
syntax from D7.This works for all callback types except
'C::foo'
.E.g. it works for
['C', 'foo']
,[new C, 'foo']
, and lambdas.And it handles references just fine.
https://3v4l.org/NiBos
In PHP 7 it also handles
'C::foo'
, but we are not there yet.https://3v4l.org/vbJRt
So, why was this changed to
call_user_func_array()
? This thread is very long, but I do not find any single mention of this function with find-in-page.Nowadays, if we are already calling
$form_state->prepareCallback($callback)
, we could make sure that'S::foo'
is translated into['S', 'foo']
.------------
Note: I only want to know about the reasons for the commit from this issue.
If we actually want to do something about it, a new issue shall be opened.