ongr-archive / connectionsbundle Goto Github PK

"Panther" name not good and need better name. It confuses with other names used in code like "Storage". So we decided to rename it with "SyncStorage".

See: https://github.com/ongr-io/ConnectionsBundle/blob/cfa7aef165069de11e0197eac86a28e0fa8d8406/DependencyInjection/Compiler/UrlInvalidatorPass.php

Is this still needed? I believe this has been replaced by pipeline model.

https://github.com/ongr-io/ConnectionsBundle/blob/65275acb568c16385e0f8d6ed263ef5a66eeb50f/Sync/DiffProvider/SyncJobs/JobsCleanupService.php#L75

There is no way to configure shop id for command ongr:sync:provide magento
while ongr:sync:execute magento.product requires shop id.
So provide puts data into ongr_sync_storage but execute expects data in ongr_sync_storage_{shop_id} .

Pipeline should be able to skip items. This can be achieved in mulitple ways:

• Throwing exception.
• Calling stopPropagation() and detecting it.

Should we throw a .skip event in that case?

code in question:

https://github.com/ongr-io/ConnectionsBundle/blob/63948a849c8befb4ddeeb4981da43438e456021a/Sync/StorageManager/MysqlStorageManager.php#L110

https://github.com/ongr-io/ConnectionsBundle/blob/63948a849c8befb4ddeeb4981da43438e456021a/Sync/StorageManager/MysqlStorageManager.php#L212

related discussion:

https://github.com/ongr-io/ConnectionsBundle/pull/81/files#r22380117

working set command looks like this:
app/console ongr:sync:provide:parameter last_sync_date --set=1 2000-01-01

--set must equal to 1 otherwise command complains that --set must be used when setting values

https://github.com/ongr-io/ConnectionsBundle/blob/master/Resources/doc/sync_storage.rst

command ongr:sync:execute [<pipeline_name>] uses these events

• ongr.pipeline.sync.execute.<pipeline_name>.source
• ongr.pipeline.sync.execute.<pipeline_name>.consume

but example gives these

• ongr.pipeline.data_sync.<pipeline_name>.source
• ongr.pipeline.data_sync.<pipeline_name>.consume

See: https://github.com/ongr-io/ConnectionsBundle/blob/8184f8cc11b004f50de115a864762f0bd643ed24/Sync/DataSyncService.php

DataSyncService name and description is too abstract and it is not clear, where this service is being used. Maybe its SyncStarter etc?

Because of this line: https://github.com/ongr-io/ConnectionsBundle/blob/master/Command/SyncStorageCreateCommand.php#L57
when creating storage, without any shop_id, storage for shop_id=0 is created, everywhere else, when no shop_id specified, default(shop_id=null) storage is created.

NEw license header should be added 2014-2015 see Content bundle.

Some classes have comments such as

Class LoadClassMetadataListener.

or incorrect definitions

Class to pass relations to compiler.

(not a compiler. There is a collection that keeps track of all relations)

Now used shop ids must be defined in parameters like this

parameters:
    shop_ids:
        - 0

But if configuration is not provided it default to shop ids and then ongr:sync:provide console command puts data in not suffixed table but SyncExecuteSourceEventListener never expect data in tables without suffix.

So the problem is that default configuration (no configuration) is invalid and it will not be clear what is wrong and i suggest either throwing errors if configuration is missing or changing default to something that works.

in Sync/DiffProvider/SyncJobs/JobsCleanupService.php, line 75

If provided by the user, the value of $this->tableName may allow an SQL injection attack. Avoid concatenating parameters to SQL query strings, and use parameter binding instead.

     *
     * @return string
     */
    protected function generateCleanupQuery()
    {
        $query = 'DELETE FROM ' . $this->tableName . ' WHERE';

        if (empty($this->shops)) {
            return $query . ' `status` = ' . SyncJob::STATUS_DONE;
        }

Also, escaping functionality exists here: https://github.com/ongr-io/ConnectionsBundle/blob/63948a849c8befb4ddeeb4981da43438e456021a/Sync/StorageManager/MysqlStorageManager.php#L86-L89. Make sure to deduplicate.

E.g. AbstractInitialSyncModifyEvent should be AbstractInitialSyncModifyEventListener.

File services.yml should be split into functionally related service sets.

A lot of logic in catch block.

Some console commands have this code:

        $output->writeln('');
        $output->writeln(sprintf('<info>Job finished in %.2f s</info>', microtime(true) - $start));
        $output->writeln(sprintf('<info>Memory usage: %.2f MB</info>', memory_get_peak_usage() >> 20));

This should be abstracted away as a CommandBenchmark object that takes $output as an argument.

There are several places, where non-strict comparison is being made with false, true or null. Change to strict equality check. See this report:

in Sync/DiffProvider/Binlog/BinlogDiffProvider.php, line 243

With booleans, only strict comparison (with === operator) should be used to lower bug risks and to improve performances.

    /**
     * {@inheritdoc}
     */
    public function next()
    {
        if ($this->valid() == true) {
            $this->setFromDate($this->current()->getTimestamp());
        }
        $this->getBinlogDecorator()->next();
    }

Posted from SensioLabsInsight

Currently, BinLog parser is using decorator pattern with nested iterators.

Team pointed out, that this is a hacky solution and should be resolved by using more flat composition architecture.

When no sql relations defined in connector, ConnectionsBundle fails, because it expects at least one relation. Should relations be required?

example relation:
https://github.com/ongr-io/MagentoConnectorBundle/blob/master/Resources/config/services.yml#L7

Most of ONGR bundles use:

<logging>
<log type="coverage-text" target="php://stdout" showOnlySummary="true"/>
<!--<log type="coverage-html" target="Tests/app/build/coverage" title="Coverage" charset="UTF-8" yui="true" highlight="true"/>-->
</logging>

for logging settings.
I think this config should be updated accordingly.

Word "execute" is reserved for "execute" process as a whole for executing elements from SyncStorage.

"start" is a good name, because pipeline might have other methods for modifying, consuming and finalizing.

There are two injections possible in class MysqlStorageManager.

see https://scrutinizer-ci.com/g/ongr-io/ConnectionsBundle/issues/master/files/Sync/Panther/StorageManager/MysqlStorageManager.php?selectedSeverities%5B0%5D=15&orderField=path&order=asc#inspectioncomment-397061209

https://github.com/ongr-io/ConnectionsBundle/blob/fd4ace90918da9731108b1f6bb11b787dfeaf4e6/Sync/DiffProvider/Binlog/BinlogDiffProvider.php

• implement getFromDate()
• implement setFromDate($fromDate)

https://github.com/ongr-io/ConnectionsBundle/blob/master/Resources/doc/sync_storage.rst

service @ongr_connections.sync.diff_provider.bin_log_diff_provider does not exist and other documentation gives @ongr_connections.sync.diff_provider.binlog_diff_provider

Investigate and exterminate.

Namespace Service is too abstract. Contents should be split into more functional namespaces like Util and UrlInvalidator.

https://github.com/ongr-io/ConnectionsBundle/blob/master/Resources/doc/sync_storage.rst

ongr_connections:
    sync_storage:
        mysql:
            connection: default
            table_name: ongr_sync_storage

should be

ongr_connections:
    sync:
        sync_storage:
            mysql:
                connection: default
                table_name: ongr_sync_storage

See: https://github.com/ongr-io/ConnectionsBundle/blob/06a95c151b4f83e6e741559e9ade18ed4f571068/DependencyInjection/ONGRConnectionsExtension.php

Main load method should be split into more smaller methods.

Page should be written, that has image of the whole import process + sync. It should include description of each stage briefly with command examples (maybe use sphinx import functionality to deduplicate code).

Possibly, Magento can be used as an example. Magento page itself will have only usage instructions. This page will be more in depth.

Further discussions are needed.

Add short and clear description, possibly copied from Resources/doc.

Add warning of alpha stability.

Add link to further documentation as in ESB.

Initiate discussion.

syntax:

$this->logger && $this->logger->...;

change to:

if ($this->logger) {
    $this->logger->...;
}

See: https://github.com/ongr-io/ConnectionsBundle/blob/a45a29c37e0a9722cec91b137c3871436f21dd2c/Event/ImportItem.php

Is ImportItem really an event? It should probably reside together with import service classes.

When Connections bundle is in vendor directory DoctrineAnnotations.php is not i the place ESDoctrineTestCase expects, thus its impossible to use this test case outside of connections bundle.

After splitting service definitions to separate files, duplicate definitions where left, few of duplications found:

https://github.com/ongr-io/ConnectionsBundle/blob/master/Resources/config/binlog.yml#L7
and
https://github.com/ongr-io/ConnectionsBundle/blob/master/Resources/config/services.yml#L99

also

https://github.com/ongr-io/ConnectionsBundle/blob/master/Resources/config/extractor.yml#L6
and
https://github.com/ongr-io/ConnectionsBundle/blob/master/Resources/config/services.yml#L106

Need to check and remove all duplications.

https://github.com/ongr-io/ConnectionsBundle/blob/master/Tests/Unit/Command/SyncProvideCommandTest.php#L16

also

https://github.com/ongr-io/ConnectionsBundle/blob/master/Tests/Unit/Command/SyncProvideCommandTest.php#L32

Your project contains files with permissive permissions. In order to avoid opening a security breach, you should restrict execution rights on following files:

• .gitignore
• ONGRConnectionsBundle.php
• Resources/config/services.yml
• composer.json

chmod a-x '.gitignore' \
    'ONGRConnectionsBundle.php' \
    'Resources/config/services.yml' \
    'composer.json'

Posted from SensioLabsInsight

There is Event folder and Pipeline/Event. We should either mover everything to Event or distribute events to their own namespaces.

I would prefer second variant to avoid crowded folders.