Problem
Issue is outcome of a problem I have in D7 + OG. I'd like to show a list of values on a select list based on the entity that the field is attached to. I can't do it now, as there is no context -- the entity isn't passed along.
Patch extracts this info which is available in options_field_widget_form() and passes it to _options_get_options().
API change
No
Comment | File | Size | Author |
---|---|---|---|
#44 | 1541792-44.patch | 717 bytes | no_commit_credit |
#44 | interdiff.txt | 689 bytes | no_commit_credit |
#43 | 1541792-43.patch | 713 bytes | no_commit_credit |
#43 | interdiff.txt | 685 bytes | no_commit_credit |
#40 | omg_i_am_so_stupid-do-not-test.patch | 693 bytes | chx |
Comments
Comment #2
amitaibuLet's test this one.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedQuick review:
$form_state[$entity_type]
. It is a common convention, but we are not enforcing it anywhere right now. Let's modify the Field API to add$element['#entity']
.$entity_type
should probably be optional too.Comment #4
amitaibu> During default editing, I doubt that we have an entity type
According to my checks it seems we always have the entity type.
> We need to update the API documentation
done.
> We probably want to update some tests
Let's see if this patch passes.
Comment #5
amitaibu> Let's see if this patch passes
Yes, it did :)
Comment #6
dwwchx and I ran into this exact same limitation when working on the D7 port of project_issue. We want a field on issue nodes (Component) that populates its allowed values via a field on the project an issue is attached to. Currently impossible without horrific hacks. This patch would enable it. Code looks good to my eyes, but it's late and I'm exhausted, so I'm not going to RTBC. But major +1 from me.
Comment #7
chx CreditAttribution: chx commentedThanks so much for the patch! But you stopped just where it started to become interesting. I continued by passing on the entity_type and entity you so nicely provided to allowed_valued_function. This allowed me to write a really purdy test.
Comment #9
chx CreditAttribution: chx commentedBoy, D8 moves fast.
Comment #11
tim.plunkettThis only added #entity for field_multiple_value_form, I think it should be in field_default_form as well.
There was also a call to
list_allowed_values($field, $instance, $field, $instance)
that I fixed.Added @param docs where appropriate.
Comment #13
tim.plunkettDuh. field_multiple_value_form() doesn't have access to the $entity.
Shoved the $entity into $form just as a rough test. Marked with @todo.
Comment #15
tim.plunkettTest had LANGUAGE_NONE in it.
Still has the hacky bit in it.
Comment #17
tim.plunkettD'oh! The test didn't enable the list module. I think this might actually work now?
Comment #18
tim.plunkettThis should be
parent::setUp(array('list', 'field_test', 'list_test'));
on the next reroll.This is likely not a good solution, but how else to get the $entity from one function to another?
Comment #19
tim.plunkettOkay, merging this with the tests that chx wrote.
Comment #20
dwwCool to see so much progress here, thanks!
It's not obvious what's going on with
$cacheable
. Can someone explain that? Perhaps via a code comment? Is the intention that$function()
might alter$cacheable
by reference if it needs to?Comment #21
dwwp.s. And if that's right, should any API docs be updated accordingly, and if so, where do those live?
Thanks,
-Derek
Comment #23
tim.plunkettAllowed values functions are poorly documented. And by poorly, I mean not at all. I will open a follow-up for better docs for those.
This adds better docs for $cacheable and cleans up the test. And adds default values for the new parameters in list_allowed_values.
Comment #24
tim.plunkettArggh. Interdiff was right, but the patches were wrong.
Comment #25
tim.plunkettThis should be a clean D7 backport. Just manually applying stuff because of docs changes or function_exists, and then swapping out LANGUAGE_NOT_SPECIFIED for LANGUAGE_NONE.
Comment #26
sunLooks good to me.
Some classes and functions are missing phpDoc, but I'd say, d.o upgrade sprint progress > nitpicks, so let's get this in.
For the D8 patch, some more details about (optional/required) function arguments, argument order, etc.pp. have been answered in previous comments already, and those will be fixed in follow-up issues specifically for D8.
Comment #28
jthorson CreditAttribution: jthorson commentedFailure was a tests-only patch ... reversing to previous status before testbot interfered.
Comment #30
chx CreditAttribution: chx commentedMoar doxygen. I rolled the D7 patch by applying the interdiff so there is no need for a D7 interdiff.
Comment #31
webchickOk, got an in-person walkthrough of this code from chx and dww. :)
Adding the additional optional properties makes sense. I was a bit thrown off by the weird cacheable by reference stuff. chx explained that this is because we can't remove a drupal_static in a stable release of Drupal, and in order to enable the D7 fix to be committed sooner, we keep D7 and D8 the same and then open a follow-up issue to clean that up. And also make those properties required.
Committed and pushed to 8.x and 7.x. Thanks so much for the fast turnaround on this, folks! :D Drupal.org thanks you.
Comment #32
chx CreditAttribution: chx commentedOpsie.
Comment #33
tim.plunkett+1 on the RTBC, this was due to a mistake I made back in #25.
Comment #34
webchickCommitted and pushed that to get testbot working again.
Sorry folks. :\
Comment #35
chx CreditAttribution: chx commentedLet me reopen this once more. We have random failures (see #30) and I am fairly sure this solves them.
Comment #36
catchCommitted/pushed to 8.x, moving to 7.x for backport.
Comment #37
chx CreditAttribution: chx commentedErm. #35 had it.
Comment #38
webchickCommitted and pushed that too. Yeesh. :P
Comment #39
dww(For posterity.)
Thanks everyone, and sorry to webchick for the troubles this patch has caused. Drupal.org D7 loves you.
Comment #40
chx CreditAttribution: chx commentedSee...... the random failures were caused by $id and $vid sometimes having the same value in ListDynamicValuesTestCase::$test then being fed into drupal_map_assoc by list_test_dynamic_values_callback resulting in 3 values instead of 4. (The 4/5th is the _none).
In other words, we are doing
drupal_map_assoc(array("randomstring",3,3,"test_bundle")));
. OOOOOPSIE!Comment #41
BerdirYes, this makes sense. Let's wait until the bot comes back, but this should be RTBC. In case you're confused like I was, the do-not-test.patch is the patch for 7.x.
Comment #42
xjmFor the record, entirely missing doxygen is not a "nitpick"; it is missing documentation and shouldn't be acceptable any more than missing tests, per the core gates:
Thanks chx for adding it.
Regarding #40, I was going to say it seemed a bit unreliable to say the value will always be less than 20, but I missed that the ID is right above it using
rand(1, 10)
. ;) So it looks fine.Comment #43
no_commit_credit CreditAttribution: no_commit_credit commentedThis is what an actual nitpick looks like. ;)
Comment #44
no_commit_credit CreditAttribution: no_commit_credit commentedOr better, this.
Comment #45
webchickAgreed w/ xjm on PHPDoc being an absolute requirement. I passed that along to chx verbally and that was taken care of before commit. :)
Thanks for the further comment clarification "no_commit_credit" ;)
Committed and pushed to 8.x and 7.x. Let's hope we're good now. :) Optimistically restoring status.
Comment #46
tim.plunkettFor those curious how this could be used, see http://drupal.org/project/dereference_list
Also, as a follow-up: #1548004: Allowed values functions are undocumented
Comment #47
webchickThis issue seems to be causing problems all over the place. :(
#1559410: Taxonomy module - Warning: Missing argument
#1559172: List module spawns error messages
I'm not sure why tests did not catch it. chx, could you take a look?
Comment #48
jthorson CreditAttribution: jthorson commentedwebchick: I hate to point this out, since I don't have an explanation for it ... but it may provide some insight to your "why tests didn't catch it" question ... that, or, it could simply be a red herring.
The patches in #43 and #44, which were posted two minutes apart from each other, do not have the same test count. This is a pattern I've been suspecting, but this one makes it easy to confirm.
Looking in the review log for the two tests, the test count for the very first category do not match (Actions in an infinite loop: 26 tests passed versus 20 tests passed). Was there a commit removing a half dozen tests in the two minutes between these, or do we have an unknown testing issue?
... thus why I hesitate to point this out. ;)
Comment #49
webchickOh, man. :\ No, no commit that should've affected those actions tests. Aye-yi-yi.
Comment #50
BerdirWhile I have noticed the inconsistent test assertions numbers as well, I don't think this is the issue here.
This is the problem.
We have changed this hook and the helper function to receive two additional, required arguments. This is an API change.
If another module is calling either the hook directly or through this helper function, stuff breaks. Then this results in the warnings in those issues. The helper function is prefixed with _, so "private", so you might say this is ok, but not the hook imho.
They should be optional, just like in list_allowed_values()
Comment #51
webchickDuh. This is why I should not commit patches at sprints. Escalating.
Comment #52
brad.bulger CreditAttribution: brad.bulger commentedre comment 50 - i can't find the relevant change, but compared to 7.12, all three additional arguments should be optional:
Comment #53
chx CreditAttribution: chx commentedThe hook gets more arguments.
function hook_options_list($field, $instance, $entity_type, $entity)
is a documentation of what arguments you are getting. If you have an old hook implementation and you desire to receive fewer arguments then nothing ever happens. That's how PHP is. You need to call a change core function to break this. What function do you call? _options_get_options is a private function. list_allowed_values is public but the new arguments are optional there.Comment #54
chx CreditAttribution: chx commentedAnd, this is hardly critical because I have yet to see an actual bug report. If contrib modules are calling hook implementations (are there any??) and private functions that's not a core bug. Give me a core bug to fix.
Comment #55
chx CreditAttribution: chx commentedAlso, the issue is extremely infuriating because now I am going to be left holding the bag just because people neither can write proper API using contrib modules nor file proper bug reports.
Comment #59
sunre: #48: @jthorson/@webchick: Nope. See you in #1560028: Remove all calls to rand() from tests
Comment #60
brad.bulger CreditAttribution: brad.bulger commentedaren't there two of them in #47? it's the specific changes to the arguments for list_options_list() and taxonomy_options_list() that are causing problems.
from #1066274: hook_options_list should allow to pass the instance of a field the new second argument, $instance, was supposed to be added as optional in 7.x. it's required in 8.x, i think, so maybe this was a backport problem. but as of 7.13 those functions only took one argument, and now they are requiring four. that seems like a legitimate backward compatibility problem.
Comment #61
dww@brad.bulger: the point is that contrib modules shouldn't be calling other module's hook implementations. taxonomy_options_list() is the taxonomy.module implementation of hook_options_list(). The only thing that should call that function is the part of options.module that invokes the hook. If other modules are invoking that function directly, they're to blame for warnings, not core itself.
Comment #62
Dave ReidHrm, no. Where have we ever said that hooks are not a public API that other modules cannot invoke?
Comment #63
dwwOther modules can do whatever they want. And if they get PHP warnings about it, they should fix them. That's not a core bug. ;) That's my point. If you're reaching into the internals of another module and invoking its hooks, you should potentially be prepared for changes. Implementing hooks invoked by other modules is a public API. However, invoking hooks on behalf of another module is not a public API, that's internal. Play with fire and you can get burned. At least, that's my take... I'm sure others will voraciously disagree with me, and that's fine.
Meanwhile, what's the bug here? I've read the other issues linked in #47 and no where is there any indication of what module(s) are actually invoking these hooks directly. The hook changed (for the better). I fail to see the core bug. If people are invoking those hooks themselves, those places need to be updated. The previous API made no sense. Higher up in the call chain we had the $entity_type and $entity, but just never bothered to pass them down the chain to this hook invocation, so many important things became impossible to implement without horrendous hacks. We're now providing enough context.
I don't think we've committed any major crimes here. And I definitely don't think reverting this change is the right solution. Nor is even making those additional arguments optional in the hook invocations. options.module decides what arguments it's passing when it invokes its own hooks. So those are going to be there when the hook is invoked. So it's pointless (and in fact, wrong) for a hook implementation like taxonomy_options_list() to declare those arguments as optional. The only point of that would be to protect against this edge case of a contrib module invoking that hook directly itself. I don't think that makes sense. If rules needs a new release to keep up with 7.14, so be it. That's what #1559172: List module spawns error messages is about. So, what else is broken?
We really do "need more info" here -- so I'm leaving the status as-is. People experiencing problems need to provide real details on what's actually broken. Yes, the internal API between options.module and taxonomy.module changed between 7.12 and 7.14. If people are getting warnings as a result, let's fix those places to keep up with progress. Please let's not say "since people are potentially doing weird things, we can't fix APIs that are broken since we might inconvenience the edge casers." which is what it seems is being implied here.
I don't want to start a flame war here or even pick a fight. I'd rather just get back to work making things better. So, if some contribs need to be patched to be squeaky-clean with 7.14, give me an issue nid and I'll try to give you a patch. Meanwhile, please don't re-break this API by reverting the fix.
Thanks,
-Derek
Comment #64
brad.bulger CreditAttribution: brad.bulger commented@dww - i see your point. it's not just direct calls, though. so i guess the question is if it's OK to break a line of code like this
(example from Entity module)
to me, that seems like an API change that should at least be initially backward compatible. which is how the initial change was implemented.
Comment #65
webchickOk, #1556192: Incorrect invocation of hook_options_list() This looks like it was coming from Entity API.
I guess setting this back to fixed, but Damien's right in that changing the hook signature on folks was not nice. I wonder if we want to make that = NULL at least in the hook documentation if nothing else.
Comment #66
dwwCool, glad to see this resolved!
No, I still don't think documenting the hook with = NULL is a good idea, as I explained in paragraph 3 in #63...
Sorry for all the troubles this issue caused! Both stress and time spent for webchick, chx's freak-out about being wrongfully blamed, end users getting confusing warnings, etc. We were just trying to make progress, but sometimes change is difficult. ;)
Cheers,
-Derek
Comment #67
joelpittet= NULL would be nicer, changing the API wasn't nice indeed.
@dww It's hard to say calling a hook from a contrib module an edge case. The release says "Includes bugfixes only (no new functionality)"
Comment #68
dgtlmoon CreditAttribution: dgtlmoon commentedIm confused, is this the fix for the PHP warning "Missing argument 2 for taxonomy_options_list()" #1559410: Taxonomy module - Warning: Missing argument that appears since i've upgraded to latest Drupal-7?
Comment #69
webchickThis is the issue that caused it. More than likely, the solution is #1556192: Incorrect invocation of hook_options_list(). Else there's another contributed module out there calling an internal API function that needs to be fixed.
Comment #70
bago CreditAttribution: bago commented@webchick: #1556192 is not yet a "solution" (proposed patches don't work yet).
Comment #71
webchickYeah, hopefully someone who uses that module and is having the problem can help debug and fix the patch over there. None of the core developers has been able to reproduce the problem as of yet.
Comment #72
Oceanman CreditAttribution: Oceanman commentedI have tried to describe how I get the errors:
http://drupal.org/node/1556192#comment-5968824
I used the patch from #1 in the above mentioned thread to get rid of the errors. FYI: I am using Drupal core 7.14
Comment #73
dynamicdan CreditAttribution: dynamicdan commentedYes I made this active again...
This fixed my problem (D7.14). My problem was that I got errors. Not that my modules were incompatible with the hook API and needed updates.
AT A MINIMUM I would expect that this problem is documented in the update notes along with "a temporary solution is xyz" (as above). Another thought would be to mark taxonomy_options_list($field) as deprecated and warn all devs to wake up and fix their modules. Still leave in the deprecated function until the key players (popular modules) have updated their modules. Would it be worth while adding a log msg like "The function 'xyz' is deprecated. Modules calling this will need to be updated before you can upgrade Drupal again".
Comment #74
tim.plunketttaxonomy_options_list is a hook. Any contrib module calling it directly probably shouldn't, and if so shoulders the responsibility of updating its calls.
The function is not deprecated.
#1556192: Incorrect invocation of hook_options_list() has already been committed to fix this in Entity API.
Comment #75
ilechcod CreditAttribution: ilechcod commentedSorry guys,
Sometimes we can get so technical with implementation details that we miss out the main point.
I'm a Drupal user, just upgraded to 7.14, and I'm getting the error
I have read 74 posts made about this error.
And simply NONE of them says: "This is the patch to use to fix this issue".
And so, after all the reading I have a Drupal site open in front of me, with my upgrade issues unsolved.
Would have so appareciated if there was a more formalized methodology of providing solutions to dedicated users of Drupal, while the tehnical talks go on.
For example, a message from "System Message" - this patch fixes the problem. Users can download and apply this patch (would also be quite helpful to mention which particular file(s) to patch) while we work on fixing this problem in newer releases.
Just some non-technical, but quite very useful suggestion.
Meanwhile, has anyone please trid any of these patches? Does any of them solve the problem (i.e stop the warning messages from showing, while the contrib modules are being fixed)?
Lot's of love...
Comment #76
dynamicdan CreditAttribution: dynamicdan commented@ilechcod see #73. add =NULL
@tim.plunkett deprecation is the wrong word. Lets just agree that better documentation is needed to help users/admins/devs relax when they do an update and see lots of warning messages. Install/update notes should be updated with a link to this issue for further advice.
Is it possible to use the update.php page to add some update related comments? Would be user friendly. Would also fit in with the info about all the other SQL based actions being made that most read and review. I know this problem is not DB related, but update is called update. Not update-db.php.
BTW, changed all the issue values.. do we instead need a new issue for this?
Comment #77
tim.plunkettSince this was originally a task that has been fixed and committed to both versions, let's leave this as is and if you feel the need, open a new issue.
Comment #78
ilechcod CreditAttribution: ilechcod commentedDan, many thanks for the quick reply.
I've looked @ #73. I'm not sure this addresses what I'm trying to say in my previous post.
For example, what do I do with this line of code?
Do I replace an existing line of code in a file somewhere? Which file exactly? Will this file be overwritten sometime in future during an upgrade?
Assume for a moment I downloaded Drupal because I'm a non-techie, and wanted a robust platform prone more to CONFIGURATIOn than PROGRAMMING. How does #73 help me?
Please don't get me wrong, I SINCERELY appreciate your posts, and the entire Drupal Community.
But if I can be really SINCERE, I still don't have tha answer to the issue (even with #73).
Please assist.
Regards
Comment #79
tim.plunkettCross post.
@ilechcod, yes you replace the existing taxonomy_options_list function declaration with that one. Look in modules/taxonomy/taxonomy.module, line 1375.
Comment #80
ilechcod CreditAttribution: ilechcod commentedSorry Tim,
You mean the fix to this peoblem has been committed to 7.14?
Or 7.14.dev?
So I can redownload 7.14.tar.gx right now, and wont have these issues anymore?
Please clarify.
Thanks
Comment #81
tim.plunkettRefresh the page before commenting, or else you'll keep changing the status.
It has been committed to 7.x-dev. It will be in the next release. Once a release is created, it is never changed, so it will not be in 7.14.
Comment #82
ilechcod CreditAttribution: ilechcod commentedOk,
Thanks
Comment #83
tim.plunkett*sigh*
Comment #84
brad.bulger CreditAttribution: brad.bulger commentedagain, that is not what was happening here. the contributed Entity module was invoking the 'options_list' hook on a list:
if you all are willing to break that line of code in order to implement a change, rather than ease it in by making new arguments optional, then that's your call.
this distinction between a "hook" being an internal thing which contributed modules should not use, and a "public API", is a new concept to me. perhaps i've misunderstood something all these years.
i thought taxonomy_options_list() was an *implementation* of the hook_options_list() hook.
Comment #85
tim.plunkettYes, I meant to say "taxonomy_options_list is an implementation of a hook".
Entity API module was invoking the hook of another module. See #1541792-63: Enable dynamic allowed list values function with additional context for a well-written explanation about how that's not core's problem.
Comment #86
Dave ReidI'm still personally baffled that hook implementations are not valid public APIs that cannot be called by other modules via module_invoke(). When did this change happen?
Comment #87
ilechcod CreditAttribution: ilechcod commented@tim.plunkett, thanks
I fixed the issue by changing line 1375 in taxonomy.module (adding the NULL values).
But I now get this error (please see below):
Apparently - it's still connected with the API change in Drupal 7.14. Any fix (even if quick) for this please?
Regards
Comment #88
ilechcod CreditAttribution: ilechcod commentedSorry guys,
I'm just trying to figure out which patch exactly to apply to my Drupal 7.14 installation.
Clicking on the patches above - information page says its for Drupal 8.x
@Berdir, the don-not-test.patch is a patch that patches the /modules/field/modules/list/tests/list.test file - is that what is required to solve this problem in Drupal 7.14 (tried it anyway - didnt change anything - still errors)
Please where is the patch for Drupal 7.14?
Regards,
Comment #89
bojanz CreditAttribution: bojanz commented@ilechcod
The only patches here are the ones that caused your problem :)
Your problem is caused by a contrib module that needs updating. Probably Entity API (you need 7.x-1.x-dev, not RC2).
I know Commerce got updated in version 1.3. Not sure if any other contribs were affected.
Comment #90
Louis Bob CreditAttribution: Louis Bob commentedUpdated Entity API module from RC2 to dev: no more errors, thanks Bojanz !
Comment #92
effulgentsia CreditAttribution: effulgentsia commentedFor people here with real world experience of what hook_options_list() and friends require, please provide some feedback on #1974444: Narrow the API of hook_options_list() and friends. Thanks!
Comment #93
yched CreditAttribution: yched commentedFYI : #2238085: [regression] options_allowed_values() signature doesn't allow for Views filter configuration questions whether we want to keep supporting that feature in D8.
See #35 / #37 over there.