Hi guys,

Thanks so much for this very useful module, it provides a very simple answer to many common use cases.

I was looking at the code of the module file and in particular, the call to db_query in block_class_preprocess_block, which brought me to the block_class.install file in which it seems the module declares a new table: block_class.

So I investigated a bit in the module's tracker to see if this topic had been discussed before and didn't find anything.

I was wondering if you ever considered actually directly modifying the block table to add a column?

That got me looking a bit further and I found i18n_block as a good example.
i18n_block does it, seems to be working very well and to be quite standard, plus I guess it's probably pretty efficient from a loading stand point.
This is just an assumption, since I haven't looked further than that into i18n_block, but I would assume that it could leverage the block caching system.

Since i18n does it, then would there be any particular reason this module shouldn't do it like that?
Code would also be greatly simplified, reduced and streamlined, along existing API objects/structures.
There might as well be some other good sides, such as not depending on foreign keys from the core table (which would prevent from having to maintain the module and delta fields, see block_class.install file functions hook_update), attaching a new property to a block to allow other modules to also interract with it based on block_load.

Most importantly, I wanted to know if there would be any good reason why it shouldn't be done like that.
Perhaps, modifying the block table is a topic or an idea that you already discussed and rejected previously.

I would greatly appreciate to have your feedback on that and if you could let me know if I overlooked or missed anything.

Feel free to let me know if you would have any questions, comments or concerns about any aspect of the discussed implementation, I would be glad to explain in more details.

Thanks very much to all, in advance, for your comments, feedback, and support.
Cheers!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zterry95’s picture

+ 1 for your idea.

It should be in this way, clean, light, easy to understand.

DYdave’s picture

Hi guys,

Thanks very much for your feedback and I'm glad to see I wouldn't be the only one interested in investigating further the suggested structural approach.

Although I didn't get much feedback/comments, I still went ahead and took a try at this implementation to see what I would come up with.

After doing quite a few tests, with Features/Features Extra, changing/enabling/disabling Themes or Blocks, or migrating data to the new database structure with hook_update, I came up with the following patch against latest block_class-7.x-1.2+3-dev (2012-10-04), attached to this ticket as:
block_class-alternate-structure-modify-block-table-1868586-2.patch

Please find below a detailed log of the changes:

Detailed log:

In general, module rewrite is greatly inspired from i18n_block:

  • Adding a field in db: block_class.install, hook_schema_alter (along with hook_install, hook_uninstall).
  • Adding a field in form: block_class.module, hook_form_alter.
  • Adding submit callback to save the field value: block_class.module, block_class_form_submit.
  • Adding class on display: block_class.module, theme_preprocess_block.



1 - block_class.install

Major change in implementation: This is the key of this implementation and where most of the structural changes would have to be made to entirely modify the database structure.

a. Hooks implementations:

  • hook_install: add the column css_class to the block table.
  • hook_uninstall: remove the column css_class from the block table.
  • hook_schema_alter: alter the block table to add the css_class field/column.

 

b. Database Update: data migration
Implementation of hook_update (see block_class_update_7103()) allows data migration from the block_css table to the css_class column in the block table, for all existing records.
If css_class column doesn't exist in the block table yet, it is created before the migration process.
The table block_css is removed after the data has been fully migrated.

2 - block_class.module

There is no major change in any functions or features of the module. Mostly the code of this file was adapted and greatly simplified to match with the new structure defined in the .install file.
Mostly, the block_class function is not needed anymore, since the css_class property added to the block table get loaded straight through block_load.
So all the calls to block_class() are replaced with $block->css_class.

  • hook_form_alter: replaced calls to block_class which simplified code.
  • block_class_form_submit: changed the query to save submitted class to the block table only if the value has changed for the css_class field.
  • theme_preprocess_block: greatly simplified the code, no need to query the block_class table anymore, since the css_class property is loaded through block_load

Last important change in this file: block_class_features_api was removed as well. See the next point for more details.

3 - Other changes (block_class.info, block_class.features.inc)

a. Cleanup (on the way):
While modifying the module, I allowed myself to change/adapt a few inline comments in all the files.
Cleaned up block_class.info file.

b. Features integration not needed anymore (See #1230234: features support ?):
This is another good side effect of staying closer to existing structural objects, in particular Drupal blocks.
Since the css_class is now implemented as a block property, then I had the pleasant surprise of finding it was also correctly exported by Features Extra's great fe_blocks module which would remove any need for any additional code in this module.
Therefore, any code related with #1230234: features support ? was simply removed in this implementation.
(Removed implementation of hook_features_api in block_class.module and the entire file block_class.features.inc)

I would like to specify this has been thoroughly tested for exporting, enabling and reverting any css_class settings exported through fe_blocks features.

Which pretty much wraps up all the changes included in the patch.

Last but not least:

Overall Results:

It is very important to specify that no new features or functionnalities have been added in this implementation.

Leveraging block_load is probably the key point in this version, which actually results from the only really significant change in the module:
Dropping the block_css table for a column/field called css_class in the core block table.

As a direct consequence, module's implementation of the css_class block property gets closer aligned with core structures, objects, functions and methods related with Core Blocks. Therefore, relying more on the core block module, brings greater compatibility with updates and evolutions, making routine maintenance easier and more sustainable (since the module and delta database keys are not stored in a separate table, see hook_update implementations in block_class.install).

Among one of the most interesting results is the enhanced compatibility provided by this approach, hence followed by all contributed modules:
Panels, Features/Features Extra, Bean, Cache, Internationalization, CCK Blocks, Context, etc... I have already tested a few modules and each time, it seems to be working as expected.

Code has been significantly reduced: about half of module's entire code base has been removed, greatly improving its readability, bringing better balance of inline comments, making it much more accessible to all developers.

To sum this up, resulting module is simpler/clearer, easier to maintain, more sustainable, more compatible and achieves better performance.

 

To ease the testing process for anyone that would be kind enough to take the time to look into this ticket, I have also attached the patched module in a zip package, ready to be extracted and tested.
File attached as: block_class-7.x-1.x-dev.zip

If you think the proposed implementation attached would help improving the module and you would consider this feature request as acceptable, I would certainly be glad to discuss further how this code/patch could be integrated to module's code base, back-port to Drupal 6.x or any additional work that would contribute to improving the overall quality of the module, for example on documentation.
Another option would be to create another/new branch 7.x-2.x, for example, which would leave the choice to developers and users to choose a preferred structural implementation (features would be the same, with or without the block_css table).

Could you please let me know if I overlooked or missed anything in the current Block Class module, in terms of whether the approach is correct or not?

Please let me know if you would have any questions on any points/code/aspects mentioned in this ticket, I would surely be glad to provide more information.

I would greatly appreciate some help from module maintainers and if any of you could take a bit of time to look into the attached patch to give me your feedback/opinion on this feature request.

Any feedback, testing, changes, recommendations would be highly appreciated.
Thanks to all in advance.

DYdave’s picture

Component: Miscellaneous » Code
Category: support » feature
Status: Active » Needs review

Changing status for code review.
Moving this from support to feature request.

Thanks very much to all for your feedback, comments, testing and reviews.
Cheers!

hackwater’s picture

Looking over the code, it's an elegant solution. Is there any objection to modifying the block table in this way?

I have a number of sites using block class, and when I found myself wanting to collect my block classes in Features, I found this and the current dev; I plan on testing out the new method (perhaps make it a 7.x-2.x-dev branch?) on dev copies of our DBs Monday or Tuesday.

hackwater’s picture

Code looks good; module installs/updates DB with no discernible change to users (blocks with classes continued work as expected).

Edited to add: Features Extras does not support menu block exports/imports yet It's likely this is actually a Features/Features Extras bug, but I thought I'd mention it in case it's actually the modified block class (I'll revert and test on the original 7.x-1.x-dev code this weekend): I can't seem to get the exported Features code containing my block classes to work with menu blocks (no machine names for menu blocks, maybe?).

To expand on that, I have a series of blocks provided by the menu module, and block class allows me to add classes to these. When I export these blocks as well as other standard blocks (all containing block classes), the classes show up in the Features code, but enabling the module created by Features to test the import/creation of these blocks results in standard blocks with classes and menu blocks that have to have classes manually added.

If this is actually a problem with Features or Features Extras, then I'd say we've got a successful review for this alternate structural approach.

DYdave’s picture

Hi hackwater,

Thank you so much for your interest and giving us your feedback on that.

I'm glad to see everything worked out as expected with the rewritten module and that you didn't have any problem other than the one related with Menu block.

Although, this is not directly related with this particular ticket or Block Class, you certainly got my attention on that and I took a closer look, because I remember that we also encountered this issue in the past.

So I allowed myself to post another ticket in the Menu block's tracker on #1894280: Exporting Menu Blocks with Features Extra and Strongarm, on which I would surely greatly appreciate to get your feedback, comments and questions.

Hopefully, this will help fixing all your issues related with exporting Menu block configurations to Features, providing a proper replacement method for the previous features support provided by Block Class's #1230234: features support ? (whose code was entirely removed from the rewritten "block_class-7.x-2.x-dev").

Feel free to let me know if you would have any other questions, comments, reviews, feedback, on any aspects of this ticket or #1894280: Exporting Menu Blocks with Features Extra and Strongarm, I would certainly be glad to provide more information.

Thanks again very much for your review and feedback, it is highly appreciated.
Cheers!

hackwater’s picture

Hi DYdave,

Actually, I didn't mean the Menu Block module, but rather the blocks generated by the core Menu module. If I build a menu, I get a block for that menu. That menu block can have a class (and various other settings); Features (and Features Extras) does not yet appear to support import/export of those block settings, as when I build a regular block with a machine name (per the fe_block module's specification), my block_class settings (and other block settings, like visibility) are exported correctly (and imported correctly when I enable the Features-created module), but my block settings for menu blocks (but not menu_block module menu blocks, as I don't use that module), such as block_class and block title and block visibility are not imported (though they appear to be in the exported code).

I suspect this is an issue/todo for the fe_block module, though, rather than a bug in your code, which is working great on my two test sites.

DYdave’s picture

Hi hackwater,

Thanks again very much for your time reviewing my answers and your prompt reply.

Thanks also for clarifying your issue related with fe_block/Core Menu modules and confirming this updated version of block_class is

is working great on my two test sites.

So I'm not going to elaborate much more on the Features Extra/fe_block/Core Menu export issue that you further described in #7, since I would like to keep this ticket focused on Block Class, as much as possible, but once again, you caught my attention on this and I did another test on my local sandbox.

I would certainly be interested in finding out more on this issue, since the results on the multiple tests I did locally, in development environment, were all successful.
With latest Drupal 7.19 stable version, I tried creating a new Menu called Footer menu (Menu ID: footer-menu), with two links to two different random test nodes (node/3 and node/5 for example). As you kindly explained, creating a new Menu would also create a new Menu Block (and not menu_block Block... understood :-) ), named by default Footer menu, that I chose to display in the Footer second column Region of the Bartik theme (Drupal 7.19, default installation).
I then went into Features to export my Block Settings (of course fe_block from Features Extra enabled), along with all the related Menu information (this assumption could probably be further questioned): footer-menu and the two links (menu-footer-menu:node/3 and menu-footer-menu:node/3, for example).

Please find attached to this comment the generated feature resulting from these several steps: menublock_export1.zip

I guess, all you would really need to do to test this feature would be to change the paths of the menu links in:
menublock_export1.features.menu_links.inc and menublock_export1.info
find/replace node/3 and node/5 with whatever path you would like (nodes or views, etc...) on a default install and this will pretty much create the menu with corresponding links, the block and all the correct configurations.

Watchout for one thing though, after enabling the feature, I found it displayed as overridden. If you check in detail, you will see only the Block Settings section is actually overridden. I checked and indeed the block wasn't appearing on the admin page, but the menu and the links were all there (edit/save and all that would work fine). So I reverted the feature's Block Settings and it seemed to work as expected after that: the block was displaying on the admin page, in the front end, in the right region, with the correct block class (our primary concern in this ticket).

You nicely pointed out the inline code comment in the fe_block module and it seems you would also

suspect this is an issue/todo for the fe_block module

(See #5 and #7)
so that also drew my attention and I tried taking a further look in Features Extra's tracker but couldn't find much related to this topic.

I would sincerely greatly appreciate if you could spend a little bit more time taking a closer look at this feature and perhaps elaborate a little bit more on the problem you described. If you are able to narrow this down a bit further, we might be able to add a new ticket in Features Extra's tracker and continue the discussion and testing there.

I would also be very happy to post more documentation on this or provide more support, since this is actually a common use case that we encounter on many projects.

I hope I was able to get your point this time and didn't get carried away again on a misunderstanding or overlooked anything, but please let me know if I missed something.
Feel free to send me a direct PM or contact me on IRC, I would be glad to test any particular feature you would have and help narrowing down what could be the causes of this issue (if this could speed up our exchanges).

I would greatly appreciate to have your feedback on all that and once again thank you very much for your time testing and sending us your comments.
Cheers!

DYdave’s picture

Priority: Normal » Major

This feature request with a status of needs review has been awaiting response from a maintainer/reviewer for more than two weeks (See #18, last updated: January 23, 2013 at 1:56pm):

Changing priority to Major as an attempt to get more attention from maintainers and other developers (special thanks to hackwater for his help testing and reporting).

If this attempt is unsuccessful after more than a week, I will try posting a request for co-maintainership and hopefully keep this ticket moving forward.
Thanks in advance for all your comments, feedback and testing.
Cheers!

DYdave’s picture

Alright, still no reply on this.
Yet, another attempt to attract maintainers' attention: #1922252: Request to take over maintainership/co-maintainership for Block Class.

Hopefully, we'll manage keeping things moving forward on this.

Thanks in advance for all your comments, feedback and testing.
Cheers!

Todd Nienkerk’s picture

@DYdave: Can you supply examples of other contrib modules that make changes to core tables?

DYdave’s picture

Priority: Major » Normal

Hi Todd,

Thank you so much for this reply, I have been longing for a review from module's maintainers.

Yes, for sure, that's actually at the foundation of this approach/idea, at the beggining of the thread, see issue summary:

I was wondering if you ever considered actually directly modifying the block table to add a column?

That got me looking a bit further and I found i18n_block as a good example.
i18n_block does it, seems to be working very well and to be quite standard, plus I guess it's probably pretty efficient from a loading stand point.

Followed by #2:

In general, module rewrite is greatly inspired from i18n_block:

  • Adding a field in db: block_class.install, hook_schema_alter (along with hook_install, hook_uninstall).
  • Adding a field in form: block_class.module, hook_form_alter.
  • Adding submit callback to save the field value: block_class.module, block_class_form_submit.
  • Adding class on display: block_class.module, theme_preprocess_block.

To find more modules examples, I did a quick query on Google to only search modules on drupalcode.org with a hook_schema_alter and found the following modules examples that all make changes to core tables:

  1. Modr8: adds a field/column to the node table, see modr8.install, line 13.
  2. File Entity Paths (fe_paths): adds a field/column to the file_managed table, see fe_paths.install, line 122.
  3. File (Field) Paths: adds a field/column to the file_managed table, see filefield_paths.install, line 13.
  4. Util/Block Tracker: adds a field/column to the block table, see block_tracker.install, line 12.
  5. Block Revisions: adds two fields/columns to the block_custom table, see block_revisions.install, line 85.
  6. Internationalization: already mentioned above, modifies Core tables all over. Here is another example with i18n_menu, adding two fields to both menu_links and menu_custom tables, see i18n_menu.install, line 36
  7. Single Sign-On (aka SSO or Single Sign On): adds three fields/columns to the sessions table, see singlesignon.inc, line 64
  8. Entity menu links: adds fields/columns to the menu_links table, see entity_menu_links.install, line 72

and I stopped looking there (lack of time, could have gone much longer...), but I'm sure I could probably pull out more modules that implement changes to the Core tables.

Please let me know if you would like me to provide more examples or would have more questions, comments, objections, recommendations or concerns on any aspects of this particular implementation or proposed patch, I would surely be delighted to provide more information and explain in more details.

If you don't have any objections with that and the implementation above looks good/consistent/solid to you, I will allow myself to go ahead and commit the proposed version in the 7.x-2.x development branch.

Once again, thank you very much for your feedback and review on that, it is certainly highly appreciated.

Thanks in advance to all for your comments, testing, feedback and reporting.
Cheers!

Note:
Attention from maintainers has been obtained, moving priority back to where it should be: normal.

DYdave’s picture

Status: Needs review » Fixed

Alright guys,

Post was initially created on December 17, 2012, so almost 4 months ago (in~10 days) and got a very positive feedback from @hackwater for the initial testing.

I've waited for 2 weeks of inactivity (afterwards), from #8 to switch post to major in #9, then another 2 weeks to post #1922252: Request to take over maintainership/co-maintainership for Block Class at #10, which finally got us an answer from @Todd Nienkerk, to which I responded at #12 and waited for two more weeks....
(Would we be the only ones really interested in this feature?)

I hope and like to tell myself that at this point, if anybody really had any objections or major concerns with this approach.... they would have spoken by now.

After a final two weeks of inactivity (couldn't wait any longer :-) ), I'm glad to let you know this feature request has been pushed to the new 7.x-2.x branch at: d995a7f.
Since @Todd Nienkerk also kindly gave me the Manage Release permissions, I went ahead and created two corresponding releases:

  • 7.x-2.x-dev: new development branch (supported) to which we will be adding new features, fixing bugs and providing support.
  • 7.x-2.0 (stable and tested thoroughly by @hackwater)

I allowed myself to mark this issue as fixed, but feel free to re-open it, or post a new ticket, at any time if you have any further objections with this approach (and we would all be very happy to hear your feedback).

We would all greatly appreciate your feedback on these new releases.

Feel free to let us know if you would have any further questions, comments, issues, feature requests, suggestions, recommendations, objections or concerns, we would surely be glad to provide support, explain in further details or give you more information on any aspects of this ticket.

Thanks a lot in advance for your testing, reporting, reviews and tickets.
Cheers!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.