It will take me a few days to look over this, but I'm going to say upfront that I'm sad to see that ACL is still scattered in both the controller and the model.

Perhaps you could explain how your envisioned extension developers overriding ACL for their extensions.

@joomdonation Yes this is true however this is something my code is forced to inherit from the base classe as they require it (see https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/view/base.php#L36)

@Mathewlenning So whilst i greatly enjoyed the allowAction method that you built - at some points it wasn't feasible to use it. For example in the create.php I need no model! So initialising a model just for ACL makes no sense! I think there are places I can clean up on where the model can be called. But if there is ACL checks in a controller I've tried to move them into a separate task where possible to make overriding easy. I'll go back and look through again though and clean up where possible (there's at least one in JControllerUpdate I've noticed I can move to the model

Do you realize how insane this statement sounds.

"For example in the create.php I need no model!"

What are you creating? Where are you creating it? How are you creating it?

I'm alway from my desk, but I'll have more specific questions when I get back.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Aug 21, 2014, at 6:16 PM, George Wilson notifications@github.com wrote:

For example in the create.php I need no model!

Do you realize how insane this statement sounds.
"For example in the create.php I need no model!"
What are you creating? Where are you creating it? How are you creating it?

George is correct that there may be tasks where you don't necessary need a model.

ACL is a funny thing when it comes together with MVC. In MVC the controller is responsible for the "Business Logic", while the model is responsible for the data interaction. ACL is actually business logic and thus belongs to the controller. But as soon as you have a bit more complex ACL it depends on the data. Like for example if a user is allowed to edit his own data or if the ACL is depending on categories. Now you need to do some ACL together with the model. And then again you need the ACL within the view as well because not every user is allowed to see everything. Like for example an edit button should only be visible for the user who is allowed to edit, or a form may not allow to edit each field for everyone.

So ACL is something which often enough is used in all three parts of MVC and there is nothing wrong with that by itself. The only important thing is to not repeat the same check in multiple locations, thus keeping it DRY. If you make the same check in the controller, the model and the view, then there is likely something wrong somewhere.

Just for the record, ACL is neither handled in the controller or in the model. It is handled in the JAccess class via the user object, but you all know this right. I have neither the time or the energy to start this argument again.

We can have this discussion in a few months, after it is merged and you actually try to build a few extension with it. Happy Joomla!ng

@Mathewlenning if you want to PR a fix for all of it to be moved into the model go for it.

If you actually want to read the create controller you can see for yourself there is no model being used at all rather than just critising without having read the code

I read the code. The create controller isn't creating anything. It is redirection to the edit form. I'm not going to get into my issues with that logic, but as I said I was away from my desk at the time.

Please don't take this personal. It is about the code, not the coder.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Aug 21, 2014, at 8:12 PM, George Wilson notifications@github.com wrote:

@Mathewlenning if you want to PR a fix for all of it to be moved into the model go for it.

If you actually want to read the create controller you can see for yourself there is no model being used at all rather than just critising without having read the code

\
Reply to this email directly or view it on GitHub.

I agree. But the ACL check for being able to create an item is different to editing an existing item. Hence why it's a different controller. Even if the end view is the same

As you know I did PR for a fix to the whole legacy mess.

It didn't work out so well. Nothing to do with the code, I just don't have the right attitude.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Aug 21, 2014, at 8:12 PM, George Wilson notifications@github.com wrote:

@Mathewlenning if you want to PR a fix for all of it to be moved into the model go for it.

If you actually want to read the create controller you can see for yourself there is no model being used at all rather than just critising without having read the code

\
Reply to this email directly or view it on GitHub.

As you can see in the JViewJsonCms class, it still depends on FOF .

As you can see in the JViewJsonCms class, it still depends on FOF .

Yeah, sorry. Didn't see you commented on a specific commit. That doesn't link well the way GitHub adds the comments here.

@joomdonation I'm happy for a specific element. As far as I'm concerned a default functionality for hypermedia json api is not exactly a cornerstone of it. It's just easy fruit. Note that as FOF is 3.x we can always "fork" in worst case the library and take the class. I think we should be able to convert the FOFInput function to something in core as well which would leave us "just" dependent on FOFDocumentHAL anyhow.

This is all still just a stop gap until Chris Davenports web services team finish their awesome work anyhow!

No problem, Thomas. I am not familar with Github, just start trying to use it recently. Not sure how to make it links correctly here.

Anyway, just want to point out that it depends on FOF and it is clear that we should not depend on it on this new MVC.

There's nothing forcing you to use FOF. If you have a layout file it will use that and never use FOF (https://github.com/joomla/joomla-cms/pull/4137/files#diff-3ff602565c0032bcf4d33d2f70107250R123) if you choose to use a default fallback I don't see why we should code a few thousand lines when we already have similar functionality in the CMS....

No problem, Thomas. I am not familar with Github, just start trying to use it recently. Not sure how to make it links correctly here.

I think if you comment on a line in the actual PR, then it links fine. If you do in a commit, then it doesn't.

So the new MVC is dependent on both legacy and FOF?

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Aug 22, 2014, at 6:03 PM, George Wilson notifications@github.com wrote:

There's nothing forcing you to use FOF. If you have a layout file it will use that and never use FOF (https://github.com/joomla/joomla-cms/pull/4137/files#diff-3ff602565c0032bcf4d33d2f70107250R123) if you choose to use a default fallback I don't see why we should code a few thousand lines when we already have similar functionality in the CMS....

\
Reply to this email directly or view it on GitHub.

@wilsonge so the require_once /fof/… is a fallback? for generating the JSON view if layout doesn’t exist?

And only if the hypermedia parameter is set in the view. I.e. we are producing HAL compliant views. But yes.

My view on FOF is that the core should be importing in classes that offer fresh and useful functionality. Such as the inflector (yes I'm using that too in JControllerUpdate). We've taken the less one from source and we're using all the FOF Encryption classes for 2-factor authentication. So I have no problems what-so-ever using FOF when it gives me functionality that isn't available in core classes when it's guaranteed to be packaged with the core for 3.x and we don't have similar code in a close to production ready state

Sure why not build our new systems on old systems and work around for the old systems.

I mean, we've been doing great with that strategy so far right.

Sincerely,
Mathew Lenning

Babel-university.com

P.S. This message was sent via iPhone, so please forgive any errors

On Aug 22, 2014, at 6:10 PM, George Wilson notifications@github.com wrote:

And only if the hypermedia parameter is set in the view. I.e. we are producing HAL compliant views. But yes.

My view on FOF is that the core should be importing in classes that offer fresh and useful functionality. Such as the inflector (yes I'm using that too in JControllerUpdate). We've taken the less one from source and we're using all the FOF Encryption classes for 2-factor authentication. So I have no problems what-so-ever using FOF when it gives me functionality that isn't available in core classes when it's guaranteed to be packaged with the core for 3.x and we don't have similar code in a close to production ready state

\
Reply to this email directly or view it on GitHub.

We probably indeed should import the classes into core if we depend on them. Like we did with JLess.

I'm not sure if you still need to include the FOF autoloader as it's now included already within the cms.php (PR #3375).

Sure I'm happy to do that if we get permission from Nic (I know technically we don't have to but...) because at least the inflector is light years ahead of anything we're using currently. I'll try and grab him over the weekend. Yes you're right. This was written fairly early on in GSOC when that didn't look like it was ever gonna be merged :P

Is there an option to work out a com_helloworld with the new MVC? It will be nice to see an actual implementation and compare to legacy or @Mathewlenning approach

This is my personal opinion: we should not add core dependencies to FOF. As I see it FOF is an abstraction layer over the core. If we start mixing things this won't end well. And that's more important if we are talking about the new MVC.

The latest formal decision of the PLT was to put FOF on maintenance mode and start removing its dependencies when possible. I've asked a formal decision of the PLT for this and the FOF update PR. Give us 2-3 days to have a formal decision.

P.S: I've been using doctrine's inflector for a long time with 0 issues.

Should we pass $this->input as a second parameter to update method of the model ? Something like $model->update($data, $this->input) .

The reason is because sometime, model need to process files upload, so it need to access to the original input from the controller . (In case we need to process file upload in model, we should not get the input again from Application object).

I'd rather inject the data from $_FILES into the model method instead of injecting the entire JInput object in. Best practices are that models should be unaware of their environment, which basically equates to they shouldn't be directly accessing the request data.

I'd rather inject the data from $_FILES into the model method instead of injecting the entire JInput object in. Best practices are that models should be unaware of their environment, which basically equates to they shouldn't be directly accessing the request data.

@mbabker Could you please explain the correct way to access to files from input and process upload in this case ? Accessing $_FILES directly mean accessing to global input while with this new MVC, each controller can have it own JInput and independent with global input ?

Really rough code example, but hopefully this conveys my thinking.

class MyControllerUpload extends JControllerCms
{
    public function execute()
    {
        // Get the file data from the input
        $file = $this->getInput()->files->get('upload', array(), 'array');

        // Basic sanity checks on data (not an empty array for example)

        // Get the model
        $model = $this->getModel();

        // Process the upload
        $model->upload($file);

        $this->setRedirect('index.php?option=' . $this->input->get('option', 'com_my'));

        return true;
    }
}

Thanks @mbabker for sample code. But in the real life, we will need to pass both the validated data and the files from input to update method of the model.

So in this case, I guess I will have to create a new controller instead of using the core controller. If I we pass the $this->input or $this->input->files as the second parameter to the update method, I will just have to override the model to processing file upload, not having to create an extra controller. Hope my explanation makes sense :).

Well that is a good first round for the code, I still need to wrap my head around how you intend this to be used. Hello Would be great, but a more functional example would also be welcome.

I'll try and follow up on all I can. com_contact example is available at in
my branch contactstuff but I need to give it a last go through to make sure
I killed off the last of the bugs and got everything finished up as well as
squashing down >400 commits :P

But it is coming

That is a lot of commits! Otukaresama. I'll take a look once its ready. =^D

Hey guys,
Just to update. I've pulled in a bundle of the minor bugs you guys were reporting - hopefully that will keep things under control.

com_contact is still in the works (sorry been a bank holiday weekend here and things have been significantly more busy than I was expecting with the family so not had much time for Joomla stuff) will try and push it out tomorrow all being well :)

Had a chat with @chrisdavenport and he said he was more than happy to move the FOF HAL class into core as it was perfectly good as a class. So to remove FOF dependency will probably do that at a later point.

Please fix the codestyle issues that Travis is complaining about.

Fixed the obvious one. There will be another that travis complains about but that's some code commented out because of a bug so that will be fixed when the bug gets fixed

@wilsonge Are we going anywhere with this PR?

Not for now