While reading the code of the context_condition class on context_condition.inc I noticed it does not follow some of the Drupal guidelines set forth for Object-oriented code.

I noticed the following issues:

  • The PHP 4-style "var" declaration is not supposed to be used.
  • All methods and properties of classes must specify their visibility: public, protected, or private.
  • Classes and Interfaces should use UpperCamel naming.
  • Methods and class properties should use lowerCamel naming.
  • Classes should not use underscores in class names unless absolutely necessary to derive names inherited class names dynamically.

Is there a sustainable reason to deviate from these guidelines? If there isn't, what would be the next steps towards refactoring this class? I think the first thing we would need to assess is how big the potential impact is to the context module. Once we identify the files and functions that we need to refactor, that list can be used as a working plan which we can check off as we commit changes. I'm available as volunteer to help with the refactoring. I'd be nice to have somebody else more experienced with Context identify what files and functions would need to be refactored.

CommentFileSizeAuthor
#3 context-1223460-2.patch78.33 KBalexander allen
#1 context_condition_inc-1223460.patch4.78 KBalexander allen

Comments

alexander allen’s picture

alexander allen commented
StatusFileSize
new context_condition_inc-1223460.patch4.78 KB

This is how context_condition.inc would look like.

alexander allen’s picture

alexander allen commented

So it looks like the bulk of the code that would need refactoring are all the default plugins declared in the plugins directory. Luckily most of the code I've seen so far only calls the execute() method, which does not need refactoring.

I'm also considering switching the visibility of other methods that are not called from outside the class to protected. Because the comments for the constructor of context_condition class indicate that it should not be overridden, I have also defined that method as final. That prevents other developers from overriding it.

alexander allen’s picture

alexander allen commented
StatusFileSize
new context-1223460-2.patch78.33 KB

Here's a bigger patch for the whole module for peer review. A couple of things:

- This patch breaks your contexts, and it should only be applied against a test environment.
- Given the size of the module and sheer amount of Objected-oriented code, I think it might be too difficult to review and test patches like these?
- However given the fact that changing the name of a class or a method touches multiple places within the module (potentially +3 files), changes cannot be limited to only the class.
- Alternate route: maybe we can refactor classes and methods one at a time?

With this patch applied to a local copy of a production environment with 15+ contexts and error reporting set to most detailed level, the module gets to show the Context admin page with no errors. However when clicking on on an individual context, no conditions or reaction show. Again, no errors appear, which is weird.

I still haven't stepped through the patched module with a debugger to troubleshoot it. I'm pausing my work on this task for now and saving my progress in the patch. I definitely anybody else to test this. I'd be nice to have other patches on top of this one covering whatever I missed, that way we can avoid duplicate work.

alexander allen’s picture

alexander allen commented
Status: Active » Closed (won't fix)

Closing (won't fix), there are higher priority issues in the queue that need to be solved first.

alexander allen’s picture

alexander allen commented
Issue summary: View changes

Minor edits