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

Refactor code and consolidate / optimize properties within traits #94

Closed
kartik-v opened this issue Sep 4, 2018 · 10 comments
Closed

Refactor code and consolidate / optimize properties within traits #94

kartik-v opened this issue Sep 4, 2018 · 10 comments

Comments

@kartik-v
Copy link
Owner

kartik-v commented Sep 4, 2018

No description provided.

@Bhoft
Copy link

Bhoft commented Sep 4, 2018

if you have the parameters e.g. _isBs4 inside the trait you have to remove them in the using classes.
Otherwise you will get issues there.

e.g.

PHP Strict Warning – yii\base\ErrorException
kartik\form\ActiveForm and kartik\base\BootstrapTrait define the same property ($_defaultIconPrefix) in the composition of kartik\form\ActiveForm. This might be incompatible, to improve maintainability consider using accessor methods in traits instead. 

But I guess you are still on it as some extensions have this values in the class directly ;)

I just realized that this issue only comes with older php version. e.g. i had tested 5.6.16 enabled.
This exception doesn't happen in my php7 version.

I had error_reporting = E_ALL in my php.ini
of course i could error_reporting = E_ALL & ~E_STRICT to get rid of this issue.

I get a similar issue with other traits.
e.g.

PHP Compile Error – yii\base\ErrorException
kartik\tabs\TabsX and kartik\base\WidgetTrait define the same property ($hashVarLoadPosition) in the composition of kartik\tabs\TabsX. However, the definition differs and is considered incompatible.

But these couldn't get rid of this error by changing the error level inside an older php version. e.g. 5.6.16.

@kartik-v
Copy link
Owner Author

kartik-v commented Sep 4, 2018

@Bhoft thanks - these have been already taken care of collectively across all extensions probably at the same time you were commenting 😄 . Do check and let know if any further issue?

@Bhoft
Copy link

Bhoft commented Sep 5, 2018

there is still an issue with at least for php version less then 7.0 (so i am not sure what you requirements are)

kartik\base\InputWidget and kartik\base\WidgetTrait define the same property ($pluginName) in the composition of kartik\base\InputWidget.

public $pluginName = '';

@kartik-v
Copy link
Owner Author

kartik-v commented Sep 5, 2018

Fixed via #95 - thanks.

@vbazovic
Copy link

I think this is the same problem: kartik\export\ExportMenu and kartik\base\TranslationTrait define the same property ($_msgCat) in the composition of kartik\export\ExportMenu. However, the definition differs and is considered incompatible. Class was composed

@kartik-v
Copy link
Owner Author

@vbazovic ensure you have the latest stable releases of all extensions with dependencies by running composer update (in your case yii2-export and its dependencies)

The updated code for ExportMenu does define the properties correctly based on trait definition.

@vbazovic
Copy link

vbazovic commented Sep 18, 2018

My first step was composer update and cache cleanup, and the error is after this. But for some reason when I looked, after your reply, in vendor folder there was an old version (1.2.8). What is strange is than on the other machine everything was OK. With identical composer.json. So after some try-error cycles I found out that the problem was missing php-gd library which then forced phpoffice/phpspreedshet to fall back to phpoffice/phpexcel which then downgraded your library to 1.2.8 if you use "@dev", as per your documentation for installing your fantastic library. So what worked for me:
sudo apt-get install php-gd
composer remove kartik-v/yii2-export
composer clearcache
composer install kartik-v/yii2-exportq1q

Perhaps you can omit "@dev" in documentation?

@kartik-v
Copy link
Owner Author

kartik-v commented Sep 18, 2018

The @dev version is an example (it can be dev-master or any version you need to install). Note its not the @dev but it is exactly the wrong dependencies that you mentioned earlier.

You have mentioned some versions of dependent extensions to yii2-export probably before yii2-export in your composer.json file like phpoffice or yii2-krajee-base or yii2-grid - you need to remove these and only have yii2-export if you want to always pick the latest version and the composer update will do that for you. If you force an extension version for the dependencies then you may end up getting outdated versions.

@danabyrd
Copy link

danabyrd commented Nov 16, 2018

Thanks for your wonderful work Mr Kartik. My project has 25 composer references to yii2-krajee-base within the vendors folder of my project. The yii2-krajee-base version references are listed below with a count for each (these counts are a sum of yii2-krajee-base references in various composer.json files in the vendor sub-folders).

>=2.0.0 (9 counts)
>=1.9   (9 counts)
>=1.9.8 (1 count)
*       (6 counts)

Can anyone suggest how I should go about resolving my dependency issues?
My plan is to start with the 1.9 entries, change one composer entry at a time, run composer update and see what happens. The issue appears to be arising from composer.json files in the vendor sub-folders, and not the primary project level composer.json file.

@kartik-v
Copy link
Owner Author

kartik-v commented Nov 17, 2018

You may want to start by reading about PHP Composer package manager to understand what you should be doing and what you should not be doing.

You are not supposed to be changing any code or files or JSON configs in the vendor folders. These are part of the vendor packages that will be automatically parsed and overwritten by composer

The purpose of using composer package manager is to avoid doing any tinkering of the source codes of vendors. You just need to manage all of your composer package versions and dependencies within Yii2 via a single composer.json file in the application root.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants