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

NDB_BVL_Instrument::factory does too much #6866

Open
driusan opened this issue Jul 23, 2020 · 1 comment
Open

NDB_BVL_Instrument::factory does too much #6866

driusan opened this issue Jul 23, 2020 · 1 comment
Labels
Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties Meta PR does something that organizes, upgrades, or manages the functionality of the codebase Proposal PR or Issue suggesting an improvement that can be accepted, rejected or altered

Comments

@driusan
Copy link
Collaborator

driusan commented Jul 23, 2020

The doc comment for NDB_BVL_Instrument::factory currently reads:

* Factory generates a new instrument instance of type
* $instrument, and runs the setup() method on that new
* instrument.

However, it does much more than that, with many assumptions about the fact that the only way
an instrument might be used is in the frontend (and not, say, to work with the instrument data in a script.)

It:

  1. Includes php files dynamically to load the instrument. (This should be replaced with a standard PHP class autoloader)
  2. Calls a setupCandidateInfoTables function (which seems to only set some smarty template variables, but as a side-effect calls Candidate::singleton and Timepoint::singleton which loads all data for a candidate and a timepoint. (This should only be done somewhere more appropriate like the handle function which is only called when accessed from the frontend.)
  3. Checks access for User::singleton and throws an exception if access is denied, (This is also assuming a front-end user and should be moved to the handle.. where it could directly return an appropriate response instead of throwing an exception..)
  4. Calls loadInstrumentFile and loadInstrumentRules. (These are no-ops for the default instrument class so probably not a big deal, but should at least be documented in the function doc)

It seems likely that setup itself will also eventually have to be renamed or removed, because of signature conflicts with the NDB_Page setup in the class hierarchy.

@driusan driusan added Meta PR does something that organizes, upgrades, or manages the functionality of the codebase Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties Proposal PR or Issue suggesting an improvement that can be accepted, rejected or altered labels Jul 23, 2020
@driusan
Copy link
Collaborator Author

driusan commented Dec 20, 2022

#8290 takes care of problem 3 but the rest are still unresolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties Meta PR does something that organizes, upgrades, or manages the functionality of the codebase Proposal PR or Issue suggesting an improvement that can be accepted, rejected or altered
Projects
None yet
Development

No branches or pull requests

1 participant