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

Add views to core #40

Closed
jenlampton opened this issue Sep 10, 2013 · 24 comments
Closed

Add views to core #40

jenlampton opened this issue Sep 10, 2013 · 24 comments

Comments

@jenlampton
Copy link
Member

cause we'll need it.

@carlwiedemann
Copy link

Really? :)

@weitzman
Copy link

Oh no, another smallcore debate :). Actually, I'm inclined to agree with @jenlampton here. +1

@davereid
Copy link

From a module perspective, being able to write admin views with VBO (#39) rather than having to hand-code every admin listing, is a huge benefit.

@rudiedirkx
Copy link

Agree. Views In Core was one of the few D8 excellencies. No more proprietary overviews!

How? Take D7's dev?

@dawehner
Copy link

If you really want to even try it: Do not remove DBTNG

#48

@quicksketch
Copy link
Member

@dawehner: Is that because the D8 version of Views leverages DBTNG heavily? AFAIK, Views in D7 is completely separated from DBTNG, it has it's own query builder and just ends up using db_query() at the end of all of it. My personal preference would be to flip the situation around and use the Views query builder if you need something alterable, and have all normal queries just be queries. It might be a crazy idea, but I'm not sure it is until I look into it.

@dawehner
Copy link

Views in D6 used to do that, but that is 100% not the case anymore in D7.

@quicksketch
Copy link
Member

Views in D6 used to do that, but that is 100% not the case anymore in D7.

Ah yep, definitely the case.

Still, the query is individually compiled all at once between views_plugin_query::build_condition, views_plugin_query::compile_fields, and views_plugin_query::execute apparently. Obviously manually building the query would be more work here, but entirely possible. The query itself is all still built within a single class, we just wouldn't have the infrastructure of DBTNG available to build the string.

This is all in the air at this point. If DBTNG doesn't actually have a performance impact, I think it's possible to keep it... though it's still Drupalism like none-other. That learning curve is what we're trying to avoid.

Are there any specific dependencies on DBTNG other than the query building that I'm entirely missing @dawehner? You're much more familiar with Views than I am.

@dawehner
Copy link

I know that there are some plugins in views which rely on using db_or() and db_and() but that is all I know of.

@alexrayu
Copy link

For me Backdrop only makes sense if is lean and fast, and has low system requirements. If it is as a hog as Drupal 8, but only without OOP in core - then makes no sense.

@quicksketch
Copy link
Member

Hi @alexrayu, removing OOP is not a stated goal, rather we want to remove complexity and abstraction where possible. Views provides a very real benefit to end-users, and it's level of abstraction is needed to deliver that feature. Drupal 7 + Views is slightly slower than D7 alone (say on the front-page view), but still twice as fast as Drupal 8 (current alpha release) serving the homepage. We're definitely going to be lighter-weight, hopefully more-so than D7, but we'll still have OOP in places where needed (e.g. plugins, cache system, config system, and views).

@DirectorHaas
Copy link

Just a refresher on this, but would grabbing the D8 version of views from around the time of the Backdrop fork (Feb 13?), or a little after make sense as a starting point? There was a point in views to D8 core dev where they had it working w/ D8 when it was pretty much D7 compatible, or before they took out the compatibility layer.

Wondering if anyone knows about the time that makes sense in the D8 Views repo to try to integrate?

@quicksketch
Copy link
Member

I've been contemplating the same thing @QuantumTime. Which would be easier: back-porting D8's Views or forward-porting D7's (or somewhere in between on an early 8.x version)?

Looking at the Git history, the official branch point for D8 Views was May 15, 2012 (d74b025). That's after our Backdrop fork on Feb 29, 2012. Almost immediately after the 8.x-3.x branch point, Views started updating to APIs that we're not likely to have at all, such as the PSR-0 namespaced tests and cache systems.

There have also been hundreds of commits to the 7.x-3.x branch since that branch point (they're also made in the 8.x-3.x branch of course), that we'd need to manually reapply if we started with an early version of the 8.x-3.x branch.

Keeping in mind our goal of as-close-as-possible compatibility, from what I've seen so far I think it's far more feasible to start with the latest 7.x-3.x branch than attempt to backport the 8.x-3.x branch (or the D8 core version). Keeping compatibility also means the following:

  • We don't rename the classes or methods to camelCase.
  • We don't change the format of stored Views in ways that we can't provide an upgrade path.
  • We don't substantially change the class structure or methods.

On the other hand we either should or must do the following:

  • Move necessarily CTools APIs into core, renaming them for general use.
  • Distribute the various Views plugins to appropriate core modules.

From an initial pass at Views module, the CTools APIs used by Views include:

  • Main APIs (used throughout Views):
    • CTools export system (which will be replaced by CMI of course).
    • CTools #dependency property and pre_render functions.
    • CTools Persistent Object Cache
  • Minor APIs (used only in specific handlers):
    • CTools Jump Menus (used in exposed filters)
    • CTools Math Expressions (used in the views_handler_field_math class)

CMI will obviously be it's own task, but I don't think it needs to hold up the Views porting job. The #dependency and persistent object caches are relatively straight-forward systems for direct porting, but they should probably be tackled first. The minor APIs can be added later or bundled together with the Views pull request.

@ghost ghost assigned matt2000 Nov 22, 2013
@matt2000
Copy link
Member

I consider this blocked by #77.

@quicksketch
Copy link
Member

Hey @matt2000, now that you've had a chance to look more into Views, do you still consider this to be blocked by #77? After all the hubbub about plugin systems (first CTools and then D8), it's easy to assume that a plugin system is required for porting Views. But as you've pointed out in #77, Views doesn't actually use a plugin system for most of code; instead it's mostly _info() hooks, with only a few systems actually using CTools plugins (as I outlined above).

In any case, the initial problems of porting a persistent object storage and #depedency can (and should) happen independently.

@quicksketch
Copy link
Member

I started this issue over the weekend to see what other problems we would encounter. Fortunately, the number of unexpected problems so far is quite low.

For starters, I merged in Views module while maintaining the entire history of the project, using the approach suggested in a StackExchange Post. This means that looking at git logs will give you an explanation and accreditation for each line of code, which can be helpful looking why things are the way the are. Unfortunately, it also means a massive addition of history, but I think the value of history outweighs the additional size here.

Conversions that are in place:

  • Switched CTools' "wizard" setup to use an info hook: hook_views_ui_wizards().
  • Temporarily moved CTools' temporary object storage directly into Views. I've made a followup issue for moving this to a dedicated subsystem here: Port CTools persistent object storage (or backport D8 equivelant) #136
  • Switched use of CTools' inclusion system to the one already provided by views (i.e. views_include instead of ctools_include).
  • Replaced loading and saving systems with CMI (so this branch is built on top of Re-merge the CMI branch #2).
  • For now, commented out all calls to the #dependency system. Instead of using it, Drupal 8 switched all calls to use #states. For the short-term, pursuing that seems the most direct and compatible approach, and it eliminates the need to port another system.

Significant pieces that are missing:

  • CTools' drop buttons are not in place. We should backport D8's #type = dropbutton element. I'll make a separate issue for that.
  • As mentioned above, #dependency does not yet work and is not replaced.
  • CTools Jump Menus and Math Expressions are not included currently.

The current branch of work is here: https://github.com/quicksketch/backdrop/compare/40;views

@matt2000, I've added you as a contributor to my sandbox if you'd like to work directly on my branch. This issue makes the merging of #2 increasingly important. Considering it's been functionally ready for weeks without much comment, I plan to merge it this week and then move forward with this issue.

@quicksketch
Copy link
Member

After digging up https://drupal.org/node/1595022 and a few additional git blames, replacing #dependency with #states was easy enough. All #states should now be working in my branch.

@matt2000
Copy link
Member

<off topic>
Sorry for disappearing. I moved into a new house last week, and was at a hackathon over the weekend. The upcoming holiday weeks have spotty patches of possible free time between family time, so maybe I can jump back in. Anyway, rest assured that I have not lost interest, and look forward to being able to help more soon.

Also, while I was out of pocket, I noticed you decided "We don't need a plugin system we should just do X, Y, and Z" where X, Y, and Z are exactly what my plugin.module + system.module patch did.... So I'm fine with the decision, as we agree on the key points, if not the terminology. It seems my very lightweight definition of "plugin" doesn't count as a plugin system to you, which is OK by me. :-)

Or maybe you just don't like the idea of "one info hook to rule them all," ('them' being "modules requiring meta-data form Classes") which is an idea I was never too strongly attached to.
</off topic>

@quicksketch
Copy link
Member

I'm very glad you're still interested @matt2000. No one likes working alone!

It seems my very lightweight definition of "plugin" doesn't count as a plugin system to you, which is OK by me. :-)

Right, in Views-land (in D7), there's very little "plugin system" involved, it's mostly just info hooks. My branch does away with the few CTools plugins that were used, mostly around the export system and new view creation (wizards). Everything else was just hooks that may have included the term "plugin" in them.

I'm actively going through the backports that Tim Plunkett and Daniel Wehner were working on some time ago (mid-2012 forward), to continue pruning legacy and necessary conversions away from CTools. Next up is CTools dropdowns, after that I'm not sure what other key areas need focus.

@quicksketch
Copy link
Member

We're pretty much wrapped up with the initial Views implementation. I converted all settings to configuration, separated Views UI into its own directory (a requirement for configuration), and included the upgrade path to move Views out of the database and into configuration. I haven't gotten around to converting the Math Expressions or Jump Menus, but we're pretty close. I'll continue working on those problems in this issue.

Here are a couple followups that will be separate issues:

@quicksketch
Copy link
Member

Here's the initial PR, though a few things still need work and tests need to be ported/fixed: backdrop/backdrop#107 (comment)

@oadaeh
Copy link

oadaeh commented Dec 31, 2013

FYI, for the yet to be created meta issue for converting listings to Views, here is the D8 meta issue: https://drupal.org/node/1823450, which also has links to some previous, tangential, and subsequent conversations.

@quicksketch
Copy link
Member

I closed the previous PR and filed a new one at backdrop/backdrop#108 that makes a cleaner pull without so much unnecessary Views history (i.e. all the other branches of Views that don't apply to us, such as 6.x-3.x).

We need to assemble a change log system (backdrop-ops/backdropcms.org#14), but until that gets sorted out I'll post a summary of important API changes made in the PR:

  • CTools "autosubmit" is now a library, "drupal.autosubmit" provided by system.module. (quicksketch/backdrop@89cb435)
  • CTools "jumpmenu" is now a library and form callback provided by system.module. Previously CTools jump menus would be generated with the function ctools_jumpmenu_form(). In Backdrop this is system_jump_menu(). (quicksketch/backdrop@fa97cfc)
  • CTools ctools_math_expr has been renamed to its original library name EvalMath and included in core/includes/evalmath.inc. (quicksketch/backdrop@741df86)
  • CTools #dependency form API properties have been removed in preference of the #states property provided by core. (quicksketch/backdrop@1b9ed86)
  • Views previously utilized CTools plugins for providing "Views wizards" for creating new views. This has been replaced with a hook, hook_views_ui_wizards.

And some other obvious changes that aren't really API changes, but significant:

  • Views now uses the config system for variables (quicksketch/backdrop@6464d91)
  • Views now uses the config system for saving, loading, reverting, and cloning views
  • Implementations of various fields, filters, sorts, etc. have been moved to individual modules instead of being grouped all together in views module directory.

@quicksketch
Copy link
Member

I've merged in backdrop/backdrop#108 for starters and filed several issues for converting listings and filed a new meta issue at #151. Thanks @oadaeh for the helpful reference!

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

No branches or pull requests