So forgive me if I'm misreading your code here, but this is what is concerning me:

  /**
  * Copies source file to the destination, making appropriate changes to its source along the way
  */
  function modify_and_copy($src, $dst) {
    $text = file_get_contents($src);
    $text = str_replace($this->src_feature, $this->dst_feature, $text);

So basically ALL matches of whatever you specify the source_feature get replaced with what you specify as the destination feature? This is very heavy handed and could cause serious issues. There needs to be some special handling here, maybe pull in each function name, rewrite the function name, and then search for the old function name and replace. Otherwise you might get unexpected results. Consider instances where the feature name might be a portion of other function names, or field names, block names, or some random string in hook_strongarm. Yikes!

Forgive me if I'm misreading or something, but if not, I would seriously recommend taking down the recommended release until this is corrected, so no-one hoses their site. :)

Comments

irakli’s picture

irakli commented
Status: Active » Postponed (maintainer needs more info)

q0rban,

you can not "hose a site" by creating source code of a new feature which won't even be enabled, just generated. With any auto-generated code there is a chance of a bug, so usual care and review is advised. But that's about it.

There is absolutely no reason why release should be brought down.

If you want to work on a smarter implementation of this part of the code - you are more than welcome to submit a patch.

Thank you

q0rban’s picture

q0rban commented

I guess I was more concerned about the ramifications of enabling the module after it's been created. I would just hate to see someone really screw up their site somehow with something like this. :)

irakli’s picture

irakli commented

Freshly generated code should be treated much like freshly written code - you assume it is buggy and test it. This module makes life easier, it's not a magic wand, neither does it replace a cautious developer.

That said:

1. I've used this module quite a bit and never ran into an actual problem
2. I am not saying existing implementation is the best possible one, but it's pretty good and in practice - good enough. Anything more sophisticated would be much more complicated/slow and the effort was just not justified when I was working on it. If somebody is willing to come up with a "smarter" solution, obviously - I will gladly accept the patch.

q0rban’s picture

q0rban commented

Freshly generated code should be treated much like freshly written code - you assume it is buggy and test it.

In an ideal world, this is true. However, I think people for the most part just trust Features. They create the code and immediately enable it. I'm not sure they would approach this module any differently.

That's all, I won't belabor the point any more. :)

q0rban’s picture

q0rban commented
Title: Holy crap, str_replace?! » Come up with a more targeted find and replace solution
Category: bug » task
Priority: Critical » Normal
Status: Postponed (maintainer needs more info) » Active

Retitling and switching to task

gagarine’s picture

gagarine commented

Create a view with a name like "my_event_list" and a feature with the name "event". Rename your feature in "events" and your view is also renamed. Now this become a problem if you call this view in an other module. It become worst if you have some custom code inside the feature with for example t('eventually') and it become t('eventsually').

You should add a note in the documentation page because it can make really nasty bug and break a lot of things.

karens’s picture

karens commented

My experience is that you just need to use names for your features that are not likely to cause problems. I wouldn't name a feature 'event', that is asking for trouble. I name all my features with complex names like 'my_event_feature' so changing from 'my_event_feature' to 'your_event_feature' should not clobber anything else along the way.

It *would* make sense to add a disclaimer to the project page that you should avoid feature names that might match content types or views names or the names of other modules and that the result of this clone is just a first pass that needs further testing on a non-production site.

sashainparis’s picture

sashainparis
Primary language French
Location Provins
commented

Of course Karen:

To say another way, as described everywhere: a Feature IS a module...
Of course, you should not name a module with the same name of another one in any project :-)

Alexandre

irakli’s picture

irakli commented
Status: Active » Closed (fixed)

Disclaimer added. Other than that: I am with KarenS on this one, overall.