Skip to content

Conversation

@Curtista
Copy link

@Curtista Curtista commented Aug 10, 2016

Summary of Changes

  • Refactor com_postinstall from FOF to Joomla core
  • Remove dependencies to FOF

Testing Instructions

  • Step 1: Go to the backend
  • Step 2: Navigate to Components->Post installation Messages
  • Step 3: All available post install messages should be shown
  • Step 4: Clicking the "Hide this message" button should hide the message
  • Step 5: Clicking a blue action button will execute appropriated action
    (e.g. 2FA Message action button will activate the 2FA plugin)
  • Step 6: If all messages are hidden a reset button will appear, clicking it will reset all messages
  • Step 7: Clicking the "Help" Button in the toolbar will popup a help window
  • Step 8: Clicking the "Options" Button in the toolbar will lead you to the component options
  • Step 9: After applying the changes Step 1-8 should work the same.

@Curtista Curtista changed the title refactor com_postistall refactor com_postinstall Aug 10, 2016
@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11546.

@mbabker
Copy link
Contributor

mbabker commented Aug 10, 2016

Unless I've missed a memo somewhere, I'm going to ask a simple question. Why?

@zero-24
Copy link
Contributor

zero-24 commented Aug 10, 2016

FOF is not maintained any more? IIRC

@zero-24
Copy link
Contributor

zero-24 commented Aug 10, 2016

I have tested this item 🔴 unsuccessfully on c27683c

It don't work for me. It looks like that the postinstall condichen method is not respected (e.g. admin_postinstall_statscollection_condition() https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_admin/postinstall/statscollection.php) check don't work.

I'm on PHP7.0.8 but it shows me the PHP5.3 warning.
same applys for the language issue since 3.4.1
and for the stats collection.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11546.

@zero-24
Copy link
Contributor

zero-24 commented Aug 10, 2016

screen shot 2016-08-10 at 14 49 01


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11546.

@rdeutz
Copy link
Contributor

rdeutz commented Aug 10, 2016

FOF is still maintained, I made a PR some months ago and it got merged into 2.x. FOF will be part of 3.x and I don't think it is flagged as depreciated for 4. So there is no real need to recode this part at this moment.

@zero-24
Copy link
Contributor

zero-24 commented Aug 10, 2016

FOF is still maintained

I'm not sure but it is up to the PLT but close the PR if will not be acceppted.

Unfortunately FOF 2 is deprecated and on its way to being completely discontinued. For over a year we've been telling developers to upgrade to FOF 3. We are not going to address any further issues in FOF 2. My recommendation is to upgrade your components to FOF 3.

akeeba/fof#593 (comment)

@rdeutz
Copy link
Contributor

rdeutz commented Aug 10, 2016

We have to ship FOF2 as long as 3.x exists, if the original author doesn't supports FOF2 we have to fill the gap

@zero-24
Copy link
Contributor

zero-24 commented Aug 10, 2016

Ok. I mean upstream support. I know we need to ship it but there is no reason to not remove the dependency from FOF2 from core components or i miss something?

@rdeutz
Copy link
Contributor

rdeutz commented Aug 10, 2016

Remove the dependency is no problem, when it works :-)

@mbabker
Copy link
Contributor

mbabker commented Aug 10, 2016

Unless you're going to refactor two factor authentication and possibly other features which are already using FOF as well, there's an inbuilt dependency to FOF code and I don't see that as being an issue right now. To me there's no benefit in going through this exercise at this point in time; it'd be the same as saying we need to use our own mail library because PHPMailer 5.2 is close to being unsupported since they're about to release 6.0. It'll have to be refactored at 4.x if a newer FOF version is shipped or FOF stops being included at all in either case, so right now it seems like a bit of premature work for the sake of doing so. That's just my opinion.

@zero-24
Copy link
Contributor

zero-24 commented Aug 10, 2016

This is the reason for this PR?

@Curtista Curtista changed the title refactor com_postinstall Refactor com_postinstall Aug 11, 2016
@Curtista Curtista changed the title Refactor com_postinstall Refactor com_postinstall from FOF to Joomla Core Aug 11, 2016
@roland-d
Copy link
Contributor

The reason for this PR is @wilsonge as he requested this to be able to deprecate FOF and remove it in the next major version of Joomla.

@Curtista
Copy link
Author

@zero-24

I fixed the issue. Could you test it again?

@laoneo
Copy link
Member

laoneo commented Aug 11, 2016

I have tested this item ✅ successfully on 6527de1

As pythagoras was the project for Joomla 4 it was intended to remove FOF joomla-x/joomla-pythagoras#66.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11546.

@zero-24
Copy link
Contributor

zero-24 commented Aug 11, 2016

Works now for me. Just one thing to check, do we know any 3Party extension that use the Core com_postinstall?

@Curtista
Copy link
Author

As long as the database entries are correct everything should work as before.

@mbabker
Copy link
Contributor

mbabker commented Aug 11, 2016

As long as the database entries are correct everything should work as before.

It's not just the database. It's the hook functions that need to be 100% B/C as well.

The reason for this PR is @wilsonge as he requested this to be able to deprecate FOF and remove it in the next major version of Joomla.

IMO this is setting bad precedent. Right now it looks like pull requests are being submitted for the sake of removing dependencies to third party code which has reached end of support. Without a proper roadmap or communication, there's no justification for this refactoring right now other than "well, we might do this later". It also opens the door to actions such as writing a Joomla mailer library to replace PHPMailer because the version we're using is no longer supported, or forking Bootstrap, or jQuery, or one of about a dozen other libraries.

There is no need to 100% refactor all code away from existing dependencies in order to deprecate it. A simple announcement of fact is all you have to do. Yes, it's preferred to refactor away, but without a roadmap there's no guidance on what should be done. Is the intent to not ship any version of FOF in 4.0? Is the intent to ship FOF 3 in 4.0? If there's an intention to deprecate FOF support in Joomla, why are we getting pull requests to first refactor the dependencies without any form of communication on why or intent? This again screams leadership fail.

@andrepereiradasilva
Copy link
Contributor

if the intention is to remove fof in joomla 4.0 this should be rebased to 4.0 branch

@Bakual
Copy link
Contributor

Bakual commented Dec 4, 2016

Afaik it was decided long ago that J4 will no longer ship with FoF.
It is perfectly fine to do this PR already in J3.

@mbabker
Copy link
Contributor

mbabker commented Dec 4, 2016

It all comes back to a subjective argument of B/C. We refactored the MVC implementation in com_config at 3.2 and a couple of developers raised issues because of its restructuring.

@anibalsanchez
Copy link
Contributor

Hi, one minor issue on labels:

  • Post-installation Messages for COM_POSTINSTALL_TITLE_JOOMLA
  • Showing messages for COM_POSTINSTALL_TITLE_JOOMLA

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/11546.

@ghost
Copy link

ghost commented Jan 10, 2017

I have tested this item ✅ successfully on 6527de1

Step 7: Clicking the "Help" Button in the toolbar popup a help window (see below) – with and without this PR.
bildschirmfoto 2017-01-10 um 17 30 49


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/11546.

@wilsonge
Copy link
Contributor

This has been merged into the 4.0 branch with 1de8d7a

Thanks for being patient with this!

@wilsonge wilsonge closed this Apr 11, 2017
@wilsonge wilsonge added this to the Joomla 4.0 milestone Apr 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.