Concerns Upgrading 5.7 with Overrides

Permalink
Hi.

I have a concern about upgrading sites that have custom overrides. I'm not sure if there is a process or mechanism for this yet, but here's my worry:

I've overridden the Form Block DB XML to add a field to a table, which also required 2 other file overrides (the controller and view). If a future release of Concrete5 modifies the concrete/blocks/form/db.xml file, to the table where I've added the field, that release / core update will be overwritten by my own override. This problem applies broadly to all overrides.

Is there any way to highlight potential conflicts to the core from override files between version x and version y of 5.7?

Thanks.

illustrationism
 
mesuva replied on at Permalink Reply
mesuva
My view on this is that you shouldn't be adding your extra fields to core database tables, it's akin to modifying core files.

Instead, I think it would be much safer to simply create a new block type, which is pretty much a clone of the the block you want to modify, then you can do anything you need to without fear of updates changing things.

Alternatively you could look at creating a new table to store your extra fields, getting your block controller to reference that when it needs that extra data. I'd be creating an extra table like that through a package, something that doesn't have to touch the core block's db xml.
illustrationism replied on at Permalink Reply
illustrationism
Ok, well maybe that's a bad example, then. But let's say a method in the controller. I do have to update the save() method for the form in order to get the new field (or field in a new table) saved. If that method gets overwritten in a core update, how would that work?

The application dir is there for overrides, and that's how the system is meant to work. If it completely breaks down for an upgrade, then what's the purpose of putting it there?
mesuva replied on at Permalink Reply
mesuva
The idea of the /applications folder is that anything in there, including your overrides, won't be touched during an upgrade. It's the same behaviour in 5.6, but just nested in the applications folder now.

In 5.6, classes like block controllers were created with empty extended classes (which would extend the 'real' class deeper within in the core. This is/was really nice in that you could just copy that and _only_ modify the specific function you wanted to, not the entire controller file. Overriding just the function you need, instead of everything in the entire controller means that if bugs are fixed in functions you haven't touched they'll get updated in an update, while your function specific override stays in place.

In 5.7, this level of indirection of class inheritance appears to have been removed. I'm guessing that there much be a way with the inclusion of namespaces where we can still just override the single function like before, instead of having to copy the entire class in as an override - I haven't worked out how to do that yet myself. (maybe someone can explain this?)
jordanlev replied on at Permalink Reply
jordanlev
You hit the nail on the head -- the layer of indirection was removed. A few of us mentioned it a while back but got no response on it. This was an incredibly useful architectural feature and I can't imagine why it was removed (my guess is "Andrew didn't think about it"). I know of no way to recreate this using namespaces or otherwise.
This is one of the big things that is keeping me from upgrading to 5.7 anytime soon. Blurgh.
JohntheFish replied on at Permalink Reply
JohntheFish
I had always assumed that the new frameworks 5.7 is built on now would provide such a sub-class override mechanism equivalent to 5.6 (my stupid blind faith). If they don't, as you say, that is a major step backwards for developers.

Either way, all these things really add up to how badly we need some developer documentation.
Shotster replied on at Permalink Reply
Shotster
> I had always assumed that the new frameworks 5.7 is built on now would provide
> such a sub-class override mechanism equivalent to 5.6 (my stupid blind faith).

Yes, that's been my experience. You simply extend a class and implement only the method(s) you need to override. It's better than the 5.6 approach IMO.

-Steve
andrew replied on at Permalink Reply
andrew
We've actually done a lot of thinking about this.

So, for things like blocks, attributes, captcha libraries, etc... we already have this. We use overrideable_core_class to sniff the filesystem, serving up the proper file automatically with the proper namespace. See my other post.

For the other items in the src directory, we have a way to enable this going forward, it just (currently) isn't implemented for every single class in the core.

The end goal is to push all instantiation of classes into the Core::make($classPath) method instead of just instantiating it directly. Then, in your root app.php you can override them and point them anywhere – you're not locked into the file system.

For example. Let's say I want to override the \Concrete\Core\Page\Page with my own custom page class for some reason. Instead of having a bunch of blank classes in the filesystem, I can just add this line into my application/bootstrap/app.php file.

Core::bind('\Concrete\Core\Page\Page', function() {
    return new \Application\Src\My\Custom\Page()
});


That custom page class would then just simply need to extend \Concrete\Core\Page\Page.

Now, there's a catch – this won't currently work today, because when we're instantiating Page we're just instantiating a class directly. It needs to go through Core::make(). But the idea is there, and we want to add support for these classes as the need arises. It also wouldn't hurt to refactor the Page class and classes that extend it for a post PHP 5.3 world.

You can currently rebind all the services (what used to be helpers) in concrete5. All service providers that create services can have those services repointed to other services that extend the original ones.

Core::bind('helper/feed', function() {
    return new \Application\Core\CustomFeedHelper();
});
mkly replied on at Permalink Reply
mkly
And unless I'm missing a bit about the concrete5 implementation you can even get really clever with your Core::bind() and do it twice where the second one overwrites the first.

Core::bind('\Concrete\Core\Page\Page', function() {
    return new \Application\Src\My\Temporary\Custom\Page()
});
$a = new SomeClass();
$a->callsAMethodThatUsesThePageClassInternally();
Core::bind('\Concrete\Core\Page\Page', function() {
    return new \Concrete\Core\Page\Page();
});


This can be really helpful and a stupid at the same time.
mhawke replied on at Permalink Reply
mhawke
The statement "add support for these classes as the need arises" frightens me. How would a developer know if you've seen fit to add support for a class they need to extend for their package?
andrew replied on at Permalink Reply
andrew
You should be able to create a form directory in application/blocks, and add a controller.php file in there, and in it place this code:

namespace Application\Block\Form;
class Controller extends \Concrete\Block\Form\Controller
{
}


You can then override any methods while still leaving the original ones intact.
mesuva replied on at Permalink Reply
mesuva
Fantastic, I thought there'd be some trick!

Thanks Andrew

(Edit - it's not really a trick, I think I'm still getting my head around the subtleties of using classes in a namespaced environment )
illustrationism replied on at Permalink Reply
illustrationism
hi Andrew. Yes, that's exactly what I've done, but what happens when that method gets updated in the core? Is there any way to see conflicts when updating?
andrew replied on at Permalink Reply
andrew
No. When updates happen it's up to you to ensure that your custom extensions still keep working. This is why we recommend making these overrides as light as possible, and unless absolutely necessary don't override heavy, brittle files like elements/header_required.php, etc...
illustrationism replied on at Permalink Reply
illustrationism
Ok, that was the answer I was looking for. Thanks.
jordanlev replied on at Permalink Reply
jordanlev
Great to hear that there is a way to override specific methods of a core class without having to copy the entire files!

@illustrationism, a technique I've been using for a while is to delete all of the empty folders at the top-level of the site (now in /applications as of 5.7), then only put them back when I am actually overriding something. In this way, I can quickly look at the directory structure and see what has been overridden. For example, if I see an "elements" directory, then it tips me off that I have a core override of an element in there. It doesn't automatically fix things when upgrades break them, but it does give me a very easy way to locate my overrides for each project.

-Jordan
jordanlev replied on at Permalink Reply
jordanlev
Oh, and speaking of clean directory structures, @andrew it seems so weird to me that 5.7 has this nice clean top-level now, but that "updates" directory is still there. Why not just put sub-folders with core version numbers inside the "concrete" directory? So upon initial installation, you have /concrete/5.7.2.1/... and then when an update happens, it creates a new /concrete/5.7.2/ directory and puts everything in there, then switches over after all files have been downloaded/extracted? It's gives the same exact benefits of having the "updates" directory, but avoids a lot of confusion when trying to give people support because there isn't this surprising place to look for core files (/concrete vs. /updates)?

-Jordan
Phallanx replied on at Permalink Reply
Phallanx
@jordanlev

Not wishing to second guess the design decision (in this case :P) I see definate advantages in the structure like it is in that when people FTP in to "clean up" directories of old versions it keeps them well away from a functioning core and they can just delete the hole upgrades dir (in theory-in practice there may be DB issues in falling back).
jordanlev replied on at Permalink Reply
jordanlev
@Phallanx, unless I'm misunderstanding you, I don't think that's desirable because if there are upgrades, the most recent version of the core is actually inside /upgrades and no longer in /concrete. Just seems kind of arbitrary to me that the first version you install is in one place and all subsequent versions are in another -- why not just have the "upgrades" directory as it works now (but call it "concrete")?

-Jordan

> On Oct 25, 2014, at 5:22 PM, concrete5 Community <discussions@concretecms.com> wrote:
Phallanx replied on at Permalink Reply
Phallanx
@jordanlev

All I am saying is I like the file isolation between the original install and subsequent upgrades. I see that the further away it is, the better (different level; thrice better).

If you are proposing that even the original install was versioned so under "concrete" you had 5.0.4 (the original install), 5.7.0, 5.7.1 etc and no other directories. I'd be for that too. However, I don't like the idea of all the different versions nestling in between the blocks, elements, single pages et. al directories just for some sense of consolodation. There is just too much to go wrong with maintenance and a sticky mouse button.
jordanlev replied on at Permalink Reply
jordanlev
@Phallanx,
Yeah I am absolutely on board with each core version being in an isolated directory. I just think those isolated directories should live under 1 top-level directory, not 2. Just seems arbitrary to me and messes up the nice clean "only 3 folders at the top-level" ideal.

-Jordan

> On Oct 25, 2014, at 10:25 PM, concrete5 Community <discussions@concretecms.com> wrote:
Phallanx replied on at Permalink Reply
Phallanx
@jordanlev

Well. They most probably do live under one.They probably live in "www.mydomain1,com", "www.mydomain2.com" etc directories off of a main one.

Whats so "clean" about 3 folders at the top level? Why not just two or one? How about 4 and include Themes because packages are at the top level?

It just strikes me as an arbitrary request for a subjective feeling of order.
jordanlev replied on at Permalink Reply
jordanlev
@Phallanx,
I think we're talking about different things here :)

> On Oct 26, 2014, at 4:05 PM, concrete5 Community <discussions@concretecms.com> wrote:
mkly replied on at Permalink Reply
mkly
@jordanlev If I understand you correctly, part of the reason may have to do with moving the concrete5 'core' to place nicer with composer. Have a look if you haven't at this discussion.

https://github.com/concrete5/concrete5-5.7.0/issues/360...

Also, to anyone here learning about Core::bind() and similar service provider type concepts, I would take a look at Pimple

http://pimple.sensiolabs.org/

It was what Symfony's IoC container was based on and Symfony's IoC is what Laravel's was based on and concrete5's is based on Laravel's.
jordanlev replied on at Permalink Reply
jordanlev
Wow, that discussion makes my head hurt :) (I do not have much experience with composer, obviously).

Nah, what I'm referring to is simply this: as of now, the directory structure looks like this:
http://cl.ly/image/3Z2m1P1v2d28...

What I'm suggesting is instead it should look like this:
http://cl.ly/image/1U0l3F3B2L39...

Everything functions *exactly* the same... it's just combining the originally-installed core and all future updates under 1 top-level directory ("concrete") instead of having the originally-installed core under "concrete" and the updates under "updates".
Phallanx replied on at Permalink Reply
Phallanx
@jordanlev.

Obviously we were not talking about different things.

You are indeed proposing a layout where I said

QUOTE>>
If you are proposing that even the original install was versioned so under "concrete" you had 5.0.4 (the original install), 5.7.0, 5.7.1 etc and no other directories. I'd be for that too.
<<

SO I'm cool with that too.
I wouldn't have been cool with (as I thought you were saying)

>applications
>concrete
>>>5.7.1
>>>5.7.2
>>>blocks
>>>elements
>packages

and..........

QUOTE>>
How about 4 and include Themes because packages are at the top level?
<<

Would have been

>applications
>concrete
>>>5.7.0
>>>5.7.1
>>>5.7.2
>packages
>themes

But themes are invariably installed as packages, so I've never really understood why there is a themes folder at all anywhere.

It's also dubious whether you need "applications" there too but I have a feeling that is a detail of the SOA framework rather than Concrete. So like I said. Subjective feeling of order.
jordanlev replied on at Permalink Reply
jordanlev
@Phallanx,
Oh, I get what you are saying now.
My logic is that the 3 directories (concrete, packages, application) are each analogous to each other... each package is a microcosm of an entire site, and they each repeat the general pattern of having /blocks, /css, /elements, etc. etc.

Whatever, obviously nobody cares about this except for me ;)

As to your question about themes outside of packages... actually I never have themes inside packages, because I just create custom themes for every client site I build (I don't use marketplace themes). I think ideally the packages are for more generic functionality that could be installed on any site, whereas the "application" directory (what used to be just the top-level of the site in 5.6 and older) is for things very specific to the one custom site you're building. But that's just my style... to each their own.
illustrationism replied on at Permalink Reply
illustrationism
Andrew: It would not be hard (and we'll have to do this for our project) to write something that compares changed files from v1 -> v2 in the core, then highlights files in the application/ dir that override them.

It doesn't have to be / can't really be granular, but at least you'll have a "map" to files you need to review. For example, if application/blocks/form/view.php doesn't change from v1 -> v2, then I can still audit it, but there's no red flag there. Whereas a change would be a flag to say "hey, you need to check out what was changed in the core and make sure your override still makes sense and works."

Thanks.
jasteele12 replied on at Permalink Reply
jasteele12
Hi Andrew,

Just tried this and get this error after submitting the form:

Class 'Application\Block\Form\Core' not found
<?php /* application/blocks/form/controller.php */
namespace Application\Block\Form;
class Controller extends \Concrete\Block\Form\Controller {
    /**
     * Users submits the completed survey.
     *
     * @param int $bID
     */
    public function action_submit_form($bID = false) { ... }
}
andrew replied on at Permalink Reply
andrew
Does the body of your action_submit_form() method use the "Core" class? If so, it needs to be imported into the current namespace by adding 'use Core;" at the top of the file, beneath the namespace declaration.
jasteele12 replied on at Permalink Reply
jasteele12
Thanks Andrew, here's what I had to actually do to get it working. Is there a better way than just trial and error for these kind of things?

use Core;
use Database;
use User;
use Config;
use UserInfo;
use Page;

Basically having to do all this just to change the Subject in the form emails (since it is not included in the mail templates [feature request])?

Now if I want to override the entire site mail helper setSubject() do I have copy the entire concrete/Core/src/Mail/Service.php to application/src/Core/Mail/Service.php ?

In 5.6 I could just do the following
class MailHelper extends Concrete5_Helper_Mail {
  public function setSubject($subject) { ... }
}


I'm not understanding how to implement the bind, and exactly what/where would I put the setSubject() function in my override?

/* application/bootstrap/app.php */
Core::bind('helper/mail', function() {
  return new ???
}