Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna’s picture

A starting point built by running the Coder module over the latest code from git (with the .info file manually. I've also attached the Coder log notes, for the benefit of anyone else who wants to look it over. The patch hasn't been tested, am going to do that now.

DamienMcKenna’s picture

Title: Drupal 7 port » Drupal 7 port of Panelizer

Changing the title so the issue doesn't get confused with the 1,000 other "Drupal 7 port" issues.

DamienMcKenna’s picture

FileSize
29.84 KB

I'm working through the errors that are coming up. Here's an updated patch with some of the minor issues resolved, but several other ones are causing problems:

  • There is no ctools_menu_set_trail_parent() function in CTools 7.x-1.x-dev so the settings sub-pages do not set the correct menu path.
  • The "Provide Default Panel" option on the main settings page does not save.
  • When you check one of the "Provide Default Panel" options and save, the item(s) that had been enabled have their links disappear on the next page load, but on subsequent page loads they re-appear.

I haven't gotten far yet, am only working through the settings pages.

DamienMcKenna’s picture

A bug in CTools v7.x that's affecting this port: #1114972: ctools_dependent_process() doesn't work in v7.x-1.x

DamienMcKenna’s picture

So that isn't a bug in CTools, the function was deprecated...

DamienMcKenna’s picture

Another issue I ran across is the following:

    $form['types']['node'][$type]['choice'] = array(
      '#type' => 'checkbox',
      '#default_value' => !empty($settings['node'][$type]['choice']),
      '#pre_render' => array('ctools_dependent_pre_render'),
      '#dependency' => array($base_id . '-status' => array(TRUE)),
      '#access' => FALSE, // Allowing a choice is curently disabled
    );

Note the '#access' => FALSE part, with the comment that says "Allowing a choice is curently (sic) disabled". Now look at the following:

      $form['types']['node'][$type]['links']['default2'] = array(
        '#type' => 'item',
        '#input' => TRUE, // necessary to fake the #process
        '#title' => theme('links', array(
          'links' => $links,
          'attributes' => array('class' => 'links inline'),
        )),
        '#id' => $base_id . '-links-default2',
        '#dependency_count' => 2,
        '#dependency' => array(
          $base_id . '-default' => array(TRUE),
          $base_id . '-choice' => array(TRUE),
        ),
      );

This then makes the dependencies structure require the 'choice' attribute to be selected, but that field is *always* hidden thus some of the links should never display. Is there a specific reason for this?

DamienMcKenna’s picture

Looking at the issue in #6, there are two blocks of links that are affected - one of them requires that the 'choice' value be FALSE, one of them TRUE. Could the reason that neither of them display be an API change that fails because the 'choice' element is not available due to the '#access' => FALSE? Just brainstorming here, I haven't dug into the code enough to know what the actual intention is yet.

DamienMcKenna’s picture

Another issue is the following line in includes/admin.inc:

  ctools_include('form');

CTools 7.x-1.x no longer has the form.inc file.

DamienMcKenna’s picture

FileSize
32.36 KB

Updated patch that swaps ctools_build_form() for drupal_build_form() and then has corrected arguments for the form functions. Still early days yet.

DamienMcKenna’s picture

FileSize
32.32 KB

Some additional small fixes including uncommenting the print() line in panelizer_default_content_page() otherwise there was no output on that page. I'd appreciate it if someone who was more familiar with CTools & Panels would take a look.

Also it seems that the issue from #6 might be a change in some of the Javascript layers. I tested the data that dependency.inc was building for the default2 links and it is identical in D7 to D6, but it behaves differently on the page.

tsvenson’s picture

Following

Shadlington’s picture

Subbing

tronathan’s picture

sub

cangeceiro’s picture

Has there been any progress to this since April 5th? I am going to need this module myself for a project and can provide some assistance finishing up this port. But it would be best to start with the most up to date version.

cangeceiro’s picture

FileSize
15.66 KB

Here is a patch that moves things a little farther forward from the patch in #10. It includes everything in that patch, also fixes a few bugs, and adds hook_node_view to actually render the panel when viewing the node. There are still some bugs to work out though. Most notibly in the admin settings. The operations links are not rendering correctly. I'm still working on that but if someone would like to chime in with some ideas im all ears.

DamienMcKenna’s picture

@cangeceiro: Thanks for continuing the work, I'll take a look at it later today and see what else I can do with it.

cangeceiro’s picture

Sounds good, wait until i post an update to the patch. I still working on it and i will have a newer version this afternoon that addresses several bugs.

cangeceiro’s picture

FileSize
43.43 KB

Ok here is the latest patch, some of these improvements should also be backported to d6. After working in this module im kinda surprised it is released as beta, personally i would say it was an alpha at best.

here is the list of updates

* Implemented rendering on hook_node_view of the display
* Moved admin menu into admin > configure > content
* Converted admin form to use d7 #states
* Cleaned up poorly written/unnecessary code
* Fixed breadcrumbs when on the admin page
* rewrote the content editor to be more abstract
* Rolled in all of DamienMcKenna work
* remembered to do a recursive diff this time ;P

and i think thats it. I'm sure there may be a few bugs creep up. but you should be able to have a function version of panelizer on d7, technically more functional then the d6 version.

cangeceiro’s picture

FileSize
43.43 KB

oops, there was a typo in the last patch. ignore it and use this one instead

DamienMcKenna’s picture

Status: Active » Needs review

@cangeceiro: awesome, thanks! I'll review it today/tomorrow and let you know how it goes.

dww’s picture

I don't yet need this myself, but I might very soon. If that's the case, I'll help test this and report my findings here.

Meanwhile, I wonder what the other maintainers think about at least creating a 7.x-1.x branch and getting this initial port into there. Seems easier to manage than a monolithic patch. @merlinofchaos + @awebb: thoughts/objections?

Thanks,
-Derek

dww’s picture

Status: Needs review » Needs work

p.s. What's up with panelizer_node_view()? That seems like a brand new feature that should be backported, not just stuffed into this porting patch/issue. Can we please separate that out into a new issue? Thanks!

cangeceiro’s picture

The code in panelizer_node_view() absolutly should be back ported. It appears that all of the configuration was in the module, but it does not render the panel when viewing the node. Thats what this code does. So the 6.x version should have this as well other wise the module is pretty much useless.

merlinofchaos’s picture

I'm in favor of creating a branch and getting code people can work with.

DamienMcKenna’s picture

Thanks to merlinofchaos and awebb I have commit access and have created a D7 branch from the patch in #19. I've also created a dev release but it'll take a little while to show up.

DamienMcKenna’s picture

Status: Needs work » Fixed
dww’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev

FYI: I just backported the code style fixes in here to the 6.x-1.x branch:
http://drupalcode.org/project/panelizer.git/commit/2ecb767
http://drupalcode.org/project/panelizer.git/commit/d85c616

@cangeceiro re: hook_node_view() -- uhh, no. The README says you're supposed to use page manager to enable the node template for your panelizer'ed node types. So, although it's a bit confusing (and not super well explained yet in the docs or UI), the D6 version works just fine if you do what you're supposed to do. Anyway, let's discuss that further at #1175890: Implement hook_node_view() to render panelizer panels display without page manager.

dww’s picture

Status: Fixed » Needs work

I just noticed that the 7.x-1.x branch has a PHP syntax error in panelizer.module. I think we can't really call this "fixed" at least until php -l passes on all the files in the directory. ;)

Also, note that I reverted the hook_node_view() change per #1175890-3: Implement hook_node_view() to render panelizer panels display without page manager.

Cheers,
-Derek

DamienMcKenna’s picture

I'll have time to work on it next week, will see what hammering I can do on it.

dww’s picture

Status: Needs work » Needs review

I already pushed a fix for the syntax error (and related code style bugs):

http://drupal.org/commitlog/commit/18014/922ee80773d5affb12ab488d784d0c6...

I still haven't even tried to setup a D7 panelizer test site yet. Let's call this 'needs review' until someone can actually verify that the 7.x-1.x branch works at all. Then we can move this back to 'fixed'. Anything beyond basic porting and getting parity with the D6 branch should happen in a separate issue.

In fact, Earl and I discussed that unfortunately ctools had to maintain its own dependency system in D7 since D7 core's #states system can't handle OR (or something). Therefore, he's not bothering to port anything to #states (just another whole system to learn and use) and panelizer clearly depends on ctools already. So, that really shouldn't have happened in this issue, but should have been proposed as a separate issue so we could have discussed it before anyone spent time implementing it (since now someone's probably going to spend time to revert those changes, too).

Thanks,
-Derek

jmones’s picture

I'm trying to install panelizer git 7.x-1.x branch and, after enabling panelizer on a particular content type, if a click on "panelizer" tab, I get the following error:

Warning: Parameter 1 to panelizer_panelize_node_form() expected to be a reference, value given in drupal_retrieve_form() (line 770 of /path/to/drupal7/installation/includes/form.inc).

Panelize it button doesn't appear.

jmones’s picture

Hi all,

I've investigated the cause of the error I reported above and it looks due to function panelizer_panelize_node_form() missing a parameter.

I attach a simple patch that seems to solve this.

Regards,

Josep

jmones’s picture

Current version is not working: view only renders node content, not content from panelizer.

Note: Please ignore this comment. I've edited later to remove it.

merlinofchaos’s picture

Views do not render content from panelizer, panelizer only works on the node page via page manager.

dww's patch to add a panelizer style to Views can solve that problem, though.

lpalgarvio’s picture

nice work here :)

DamienMcKenna’s picture

The D7 branch currently shows the settings form correctly and I'm working through the different settings, fixing things as I go. There are lots of simple things that just don't work correctly, like resetting a node's settings to the default doesn't work, hopefully by the end of today I'll have something more stable.

DamienMcKenna’s picture

DamienMcKenna’s picture

With the current codebase, when you save a content type's settings page (http://example.com/admin/config/content/panelizer/node/[type]/settings) for the first time it gives the following errors:

Notice: Trying to get property of non-object in panels_save_display() (line 863 of sites/all/modules/contrib/panels/panels.module).
Warning: Invalid argument supplied for foreach() in panels_save_display() (line 863 of sites/all/modules/contrib/panels/panels.module).
Notice: Trying to get property of non-object in panels_clear_cached_content() (line 100 of sites/all/modules/contrib/panels/includes/plugins.inc).
Warning: Invalid argument supplied for foreach() in panels_clear_cached_content() (line 100 of sites/all/modules/contrib/panels/includes/plugins.inc).
Notice: Trying to get property of non-object in panels_save_display() (line 922 of sites/all/modules/contrib/panels/panels.module).
Notice: Trying to get property of non-object in panelizer_export_save_callback() (line 452 of sites/all/modules/contrib/panelizer/panelizer.module).

This stems from the following line:

    ctools_export_crud_save('panelizer_defaults', $panelizer);

When this executes, the $panelizer object includes the $panelizer->display object, which is a panels_display object.

DamienMcKenna’s picture

I moved the most recent issue into #1213998: Error saving content type settings the first time as it affects both D6 and D7.

DamienMcKenna’s picture

Status: Needs review » Needs work

I worked out the problem with #1213998: Error saving content type settings the first time and would appreciate someone take a quick look.

DamienMcKenna’s picture

DamienMcKenna’s picture

merlinofchaos’s picture

Status: Needs work » Closed (fixed)

There's an issue for "stable release blockers". Let's use that rather than this. Since there's a port checked in, let's go with this.