Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(composer): Include PHP libraries using composer #3482

Closed
1 task done
Sidsector9 opened this issue Jul 16, 2018 · 7 comments
Closed
1 task done

fix(composer): Include PHP libraries using composer #3482

Sidsector9 opened this issue Jul 16, 2018 · 7 comments
Assignees

Comments

@Sidsector9
Copy link
Contributor

Sidsector9 commented Jul 16, 2018

User Story

As a developer, I want Give Core to include libraries using Composer instead of manually adding it into the libraries folder so that there are no chances of plugin conflicts.

Composer has it's own way to handle namespaces even for libraries which do not have their own namespaces. The motive for creating this issue was the issue that occurred in PDF-Receipts, where the plugin WP DSGVO Tools uses the TCPDF library, same as Give. Using if ( ! class_exists( 'CLASS_NAME' ) ) is tedious as the errors are in succession due to multiple classes being repeated. This solution is not feasible.

Current Behavior

Existence of the current TCPDF class and other classes inside include/libraries/tcpdf causes fatal error.

Expected Behavior

I expect that no fatal error occurs due to multiple declaration of the same classes.

Possible Solution

  1. Delete the folder tcpdf from within includes/libraries/
  2. Run composer require tecnickcom/tcpdf
  3. Add require_once GIVE_PLUGIN_DIR . 'vendor/autoload.php'; somewhere in the Give Core

Tasks

  • Use composer to include the TCPDF library and others if necessary and possible.
@Sidsector9
Copy link
Contributor Author

Sidsector9 commented Jul 27, 2018

Version Size Extra information
TCPDF (Give) 6.2.13 8.6MB The font folder consumes 6.6 MB
TCPDF (Composer) 6.2.17 (latest) 32.3MB The font folder consumes 28.3 MB

We can reduce the size of the plugin by reducing the size of the font folder. Read more


3/98 repositories under WordImpress depend on the TCPDF library, they are:

  1. Give-Gift-Aid
  2. Give-PDF-Receipts
  3. Give-Tributes

If we take care of the fonts folder, then we can include both the libraries for a certain period of time until we slowly remove the version inside the libraries folder.

CC: @DevinWalker @ravinderk @kevinwhoffman

@DevinWalker
Copy link
Member

@Sidsector9 can we via composer only include the TCPDF files necessary as described in that StackOverflow article? Otherwise, each time we use composer update then we'll have to remove those files manually, which can be prone to errors and tedious.

@Sidsector9
Copy link
Contributor Author

@DevinWalker , I don't think the client can decide to include specific dependencies. However, we can write a task for production to delete unnecessary files and folder inside TCPDF. What do you say?

CC: @ravinderk

@DevinWalker
Copy link
Member

@Sidsector9 the only issue I see with that is if TCPDF changes directory structures and we run npm run production it could cause issues if we don't catch it.

@Sidsector9
Copy link
Contributor Author

@DevinWalker I see your point, so I researched and analysed the previous versions: 6.2.10 - 6.2.17, the folder structure remains same. And in our case, the folder of focus which takes most of the space is the fonts folder, we can leave the other files as it is as they don't affect the overall size significantly. In case if the deletion task fails to delete files which are not present, we will be informed while running the task.

@DevinWalker
Copy link
Member

DevinWalker commented Jul 31, 2018

@Sidsector9 please proceed with this and updating the dependent add-ons with the

Uses TCPDF via Composer:

  • Give-Gift-Aid
  • Give-PDF-Receipts
  • Give-Tributes

Additional Questions

Which of the remaining libraries in the includes/libraries directory can we move over to composer?

2018-07-31_14-46-57

I think most of these are used by core, but can you review which may be used by add-ons?

@impress-org impress-org deleted a comment from ravinderk Jul 31, 2018
@Sidsector9
Copy link
Contributor Author

Sure @DevinWalker, once I take care of the above 3 addons, I'll research on the other libraries that can be included via Composer in Give Core.

DevinWalker pushed a commit that referenced this issue Aug 7, 2018
fix(composer): Include PHP libraries using composer #3482
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

No branches or pull requests

3 participants