-
Notifications
You must be signed in to change notification settings - Fork 5
getType(): flatten named parameters before calling init() #13
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
Conversation
|
See also #11 and horde/turba#21. |
2079075 to
9e0d4a5
Compare
a176f9b to
b683314
Compare
b683314 to
02d0a5d
Compare
|
@TDannhauer Please merge. |
|
See also #11 and horde/turba#21. |
|
hi @amulet1 , is your proposal following approach A) or B) from the issue? I suggest we have a short call - maybe tomorrow - to discuss the approaches , pros and cons to select a road to go. |
|
I chose approach (A) due to several reasons:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is the link logic now located if not longer in this variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LinkVariable is basically a refactoring of Horde_Form_Type_link.
The rendering logic is (oddly) in https://github.com/horde/Core/blob/FRAMEWORK_6_0/lib/Horde/Core/Ui/VarRenderer/Html.php. I am actually planning to move/migrate it to V3 classes (this was stalled but hopefully it can be revived right after the next beta release). This would actually make things easier to maintain.
Note, V3 classes are not used anywhere yet (except for my test setup). When this PR is updated, I will ypdate the V3_variable branch accordingly to help with V3 testing. However, at this time we should focus on making all the recent fixes available to everyone. Refactoring and major code improvements should be our second priority.
|
hm I see my folder names via Exchange ActiveSync being suffixed with [1] - might this be a consequence of the changes as well @amulet1 ? edit: visible just in MS outlook - might be a client issue |
Changes in the PR should not break anything existing. |
This is a proposed fix to #12:
I made
getType()method private as it looks like this is not used anywhere outside ofHorde_Form. And this is the only place where the named parameters could be passed (there is also two direct calls toinit()inHorde_Form_Type_datetime, but these are internal and they use positional parameters already).I am using
paramsfrom value returned byabout(). I had to adjust it in a few places (non-critical change) to match original parameters names.Later we could also add support for defaults by adding them to array returned by
about()(and should not this method be static?). It is also possible to implement parameters type checks.With the migration to
V3this will all be reworked, anyway.