Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop passing exportMode, componentTable and ids by reference in export hook #15302

Merged
merged 2 commits into from
Sep 15, 2019

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented Sep 13, 2019

Overview

As per discussion on a recent export ticket it was discovered that its practically impossible to alter exportMode, ComponentTable and the ids array according to @MegaphoneJon #15266 (comment) and backed up by @scardinius #15266 (comment)

ping @MegaphoneJon @eileenmcnaughton @scardinius

@civibot
Copy link

civibot bot commented Sep 13, 2019

(Standard links)

@civibot civibot bot added the master label Sep 13, 2019
@MegaphoneJon
Copy link
Contributor

I would add $exportMode to this list. I also neglected to add $ids to the list of arguments that would trigger a CRM_Core_Error::deprecatedFunctionWarning() if modified.

@seamuslee001
Copy link
Contributor Author

done @MegaphoneJon

@seamuslee001 seamuslee001 changed the title Stop passing componentTable and ids by reference in export hook Stop passing exportMode, componentTable and ids by reference in export hook Sep 13, 2019
@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@MegaphoneJon
Copy link
Contributor

I reviewed this from both a code and r-run perspective, and have no issues. This change requires documentation, which I just submitted.

@totten
Copy link
Member

totten commented Sep 14, 2019

Regarding this explanation:

As per recent Export discussions it seems we don't need to pass these 2 variables by reference

r-explain is generally pretty relaxed - if the substantive explanation is in a link to Gitlab or StackExchange or some wiki, that's OK. However, IMHO, the above text is too ambiguous. Put yourself in the shoes of an extension-author reading the release notes/change log and viewing this page. It would seem like we were removing functionality from the hook (e.g. can no longer alter the ID list) for no reason. It just raises a bunch of questions that the reader can't answer. To wit:

Why don't we need to pass that by reference? Is it because it never really worked? Maybe the old code was over-promising/ill-conceived? Or maybe it worked, but it became irrelevant because of some other change? Or maybe we realized that the functionality causes more harm than good? Or we found that there was no person (in MM) and no code (in universe) which actually relied on it? If I want to find out what the reason is... which discussion should I read? How do I find it? Search Google? Search Mattermost? Search Gitlab? Search Github PR? Should I even bother looking for the discussion - will it give me a clear explanation, or will it take 40 minutes to read the back-and-forth among different POVs?

I'm not saying that this particular issue needs a long explanation - but, generally, r-explain does require some explanation.

@seamuslee001
Copy link
Contributor Author

@totten i have fleshed out the description a bit

@totten
Copy link
Member

totten commented Sep 14, 2019

Cheers @seamuslee001 👍

@eileenmcnaughton eileenmcnaughton merged commit 5e15358 into civicrm:master Sep 15, 2019
@eileenmcnaughton eileenmcnaughton deleted the export_hook_alter_sig branch September 15, 2019 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants