Modules preparation : move all SQL-logic into the managers #1215

Closed
wants to merge 41 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

kanduvisla commented Mar 28, 2012

This pull requests moves all the SQL-logic made to tbl_pages, tbl_fields and tbl_sections into their corresponding Managers. I had to add some static functions to the managers to provide the needed functionality on some points, but (nearly) all the logic is now moved.

Some points of attention:

  • There is still some section-SQL-logic left in the FieldManager.
  • Also there is still some pages-SQL-logic left in the navigation datasource.

Don't worry, it's safe

I've worked and merged with the latest integration branch, and no issues have been found. This is not very surprising, since the only thing I've done is moved some logic to the managers and created some new functions in the managers to handle certain logic. In other words: nothing new is created, it's just moving of code.

I'll work further on this code, but know that it's safe to merge this pull request in the integration branch, and please consider to do so. I will continue my work on Storing Sections and Pages as XML and this pull request covers step 2-4 from my roadmap

Contributor

alpacaaa commented Mar 28, 2012

Looks great!

Owner

brendo commented Mar 28, 2012

All new functions need to be documented to include the @since Symphony 2.3 comment.

I'll review later this weekend. Something I will be looking at it trying to minimise the number of Manager functions that are quite specific and rolling them into more general functions. This will/may require some other code changes, but I think it's far better than having 'x' number of specific methods.

Contributor

kanduvisla commented Mar 29, 2012

@brendo I also tried to use existing manager functions where possible, but some cases where too specific to do so. If you could change some of the code to be more general, that would be great. As for the @since Symphony 2.3-comment: is this something you'll pick up this weekend (along with the code generalization)? Or do I have to add these lines and commit it?

Owner

brendo commented Mar 29, 2012

I can add them in as I go :) Yeah, the majority of the new functions will be needed as there's no equivalent or the workaround is horrible.

kanduvisla added some commits Mar 29, 2012

- Created Lookup Module (for lookup tables)
- Adjusted Page Manager to work with Lookup Module (part one)
- added check to work with manually created or deleted pages
- bugfix: when no page type was set, Symphony would throw an error
- bugfix: Lookup::save() didn't return the correct ID
- bugfix: When sorting the index, duplicate values got overwritten
removed addPageTypesToPage()-function and deletePageTypes() since pag…
…e types are no longer stored in a second column in the database, but in one XML-file
Owner

brendo commented Apr 4, 2012

Ok I've merged in these commits into 476aecc:

This is not a suitable replacement, e70a142 added FieldManager::fetchSchema but it only returns an array of ID's, not basic schema information.

Owner

brendo commented Apr 4, 2012

Closing. Field & Section logic will be reviewed in the leadup to 2.5 (focusing on the exciting Pages work for 2.4 first!)

@brendo brendo closed this Apr 4, 2012

Contributor

kanduvisla commented Apr 4, 2012

Wait, what?

But this is only about removing Database-logic to their specific managers! The whole idea is that extension developers use PageManager, SectionManager and FieldManager static functions to get their data instead of directly querying the database! By not providing this functionality in their Managers you risk having extensions who won't work once the Modules-concept starts kicking in!

Please reconsider

@brendo brendo reopened this Apr 4, 2012

Owner

brendo commented Apr 4, 2012

Ugh you're right. That's my cue to get some sleep!

Owner

brendo commented Apr 6, 2012

Ok have merged in the following FieldManager changes into a2b50f6

  • 3251574 (renamed to isFieldUsed for consistency)
  • e70a142 (logic is now included in an improved FieldManager::fetchFieldIDFromElementName)
  • 63e483a (renamed to fetchFieldsSchema for consistency. This function really can call a couple of places home, for brevity, FieldManager will do)
  • 670261e

Ignored these:

  • 3f722ec (unnecessary, FieldManager::fetchFieldIDFromElementName + FieldManager::fetch does the same job)
  • 8622d09 (not required, FieldManager::fetchFieldIDFromElementName does the job)
  • 4526c3c (not required, loading with FieldManager::fetch works)
  • 11c9ffb (too bespoke, not useful to any extensions)
  • f407856 (unnecessary performance impact, not needed for extensions)

Now can close :)

@brendo brendo closed this Apr 6, 2012

Contributor

kanduvisla commented Apr 6, 2012

On a sidenote:

Make absolutely sure this get announced when releasing 2.3. This way, extension developers can:

  • change their direct database calls to Manager-calls.
  • provide requests for missing Manager-functions for some specific functionality.
Owner

brendo commented Apr 7, 2012

Yep this has been added to the Migration Guide (need to flesh out a little further which I'll do when I add the release notes) and when the new API docs are generated we can scrape the 'new to 2.3' functions out to give them more visibility as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment